Merge branch '21535-multi-wf-delete'
[arvados.git] / services / api / test / unit / container_request_test.rb
index 9b35769ef2f0886ab3e1595d280b0a9a133fb83e..fa7910d597de2d0f38e8a24c56683503d37257e4 100644 (file)
@@ -34,8 +34,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
   def lock_and_run(ctr)
       act_as_system_user do
-        ctr.update_attributes!(state: Container::Locked)
-        ctr.update_attributes!(state: Container::Running)
+        ctr.update!(state: Container::Locked)
+        ctr.update!(state: Container::Running)
       end
   end
 
@@ -107,16 +107,20 @@ class ContainerRequestTest < ActiveSupport::TestCase
     {"runtime_constraints" => {"vcpus" => 1}},
     {"runtime_constraints" => {"vcpus" => 1, "ram" => nil}},
     {"runtime_constraints" => {"vcpus" => 0, "ram" => 123}},
-    {"runtime_constraints" => {"vcpus" => "1", "ram" => "123"}},
+    {"runtime_constraints" => {"vcpus" => "1", "ram" => -1}},
     {"mounts" => {"FOO" => "BAR"}},
     {"mounts" => {"FOO" => {}}},
     {"mounts" => {"FOO" => {"kind" => "tmp", "capacity" => 42.222}}},
     {"command" => ["echo", 55]},
-    {"environment" => {"FOO" => 55}}
+    {"environment" => {"FOO" => 55}},
+    {"output_glob" => [false]},
+    {"output_glob" => [["bad"]]},
+    {"output_glob" => "bad"},
+    {"output_glob" => ["nope", -1]},
   ].each do |value|
     test "Create with invalid #{value}" do
       set_user_from_auth :active
-      assert_raises(ActiveRecord::RecordInvalid) do
+      assert_raises(ActiveRecord::RecordInvalid, Serializer::TypeMismatch) do
         cr = create_minimal_req!({state: "Committed",
                priority: 1}.merge(value))
         cr.save!
@@ -127,9 +131,9 @@ class ContainerRequestTest < ActiveSupport::TestCase
       set_user_from_auth :active
       cr = create_minimal_req!(state: "Uncommitted", priority: 1)
       cr.save!
-      assert_raises(ActiveRecord::RecordInvalid) do
+      assert_raises(ActiveRecord::RecordInvalid, Serializer::TypeMismatch) do
         cr = ContainerRequest.find_by_uuid cr.uuid
-        cr.update_attributes!({state: "Committed",
+        cr.update!({state: "Committed",
                                priority: 1}.merge(value))
       end
     end
@@ -138,7 +142,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
   test "Update from fixture" do
     set_user_from_auth :active
     cr = ContainerRequest.find_by_uuid(container_requests(:running).uuid)
-    cr.update_attributes!(description: "New description")
+    cr.update!(description: "New description")
     assert_equal "New description", cr.description
   end
 
@@ -147,7 +151,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
       cr = create_minimal_req!(state: "Uncommitted", priority: 1)
       cr.save!
       cr = ContainerRequest.find_by_uuid cr.uuid
-      cr.update_attributes!(state: "Committed",
+      cr.update!(state: "Committed",
                             runtime_constraints: {"vcpus" => 1, "ram" => 23})
       assert_not_nil cr.container_uuid
   end
@@ -164,7 +168,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
   test "Container request commit" do
     set_user_from_auth :active
-    cr = create_minimal_req!(runtime_constraints: {"vcpus" => 2, "ram" => 30})
+    cr = create_minimal_req!(runtime_constraints: {"vcpus" => 2, "ram" => 300000000})
 
     assert_nil cr.container_uuid
 
@@ -175,7 +179,9 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     cr.reload
 
-    assert ({"vcpus" => 2, "ram" => 30}.to_a - cr.runtime_constraints.to_a).empty?
+    assert_empty({"vcpus" => 2, "ram" => 300000000}.to_a - cr.runtime_constraints.to_a)
+
+    assert_equal 0, Rails.configuration.Containers.DefaultKeepCacheRAM
 
     assert_not_nil cr.container_uuid
     c = Container.find_by_uuid cr.container_uuid
@@ -186,7 +192,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal({}, c.environment)
     assert_equal({"/out" => {"kind"=>"tmp", "capacity"=>1000000}}, c.mounts)
     assert_equal "/out", c.output_path
-    assert ({"keep_cache_ram"=>268435456, "vcpus" => 2, "ram" => 30}.to_a - c.runtime_constraints.to_a).empty?
+    assert ({"keep_cache_disk" => 2<<30, "keep_cache_ram" => 0, "vcpus" => 2, "ram" => 300000000}.to_a - c.runtime_constraints.to_a).empty?
     assert_operator 0, :<, c.priority
 
     assert_raises(ActiveRecord::RecordInvalid) do
@@ -215,7 +221,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_operator c1.priority, :<, c2.priority
     c2priority_was = c2.priority
 
-    cr1.update_attributes!(priority: 0)
+    cr1.update!(priority: 0)
 
     c1.reload
     assert_equal 0, c1.priority
@@ -231,11 +237,12 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     act_as_system_user do
       Container.find_by_uuid(cr.container_uuid).
-        update_attributes!(state: Container::Cancelled)
+        update!(state: Container::Cancelled, cost: 1.25)
     end
 
     cr.reload
     assert_equal "Final", cr.state
+    assert_equal 1.25, cr.cumulative_cost
     assert_equal users(:active).uuid, cr.modified_by_user_uuid
   end
 
@@ -249,8 +256,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
-      c.update_attributes!(state: Container::Locked)
-      c.update_attributes!(state: Container::Running)
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
       c
     end
 
@@ -260,13 +267,15 @@ class ContainerRequestTest < ActiveSupport::TestCase
     output_pdh = '1f4b0bc7583c2a7f9102c395f4ffc5e3+45'
     log_pdh = 'fa7aeb5140e2848d39b416daeef4ffc5+45'
     act_as_system_user do
-      c.update_attributes!(state: Container::Complete,
+      c.update!(state: Container::Complete,
+                           cost: 1.25,
                            output: output_pdh,
                            log: log_pdh)
     end
 
     cr.reload
     assert_equal "Final", cr.state
+    assert_equal 1.25, cr.cumulative_cost
     assert_equal users(:active).uuid, cr.modified_by_user_uuid
 
     assert_not_nil cr.output_uuid
@@ -297,8 +306,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
-      c.update_attributes!(state: Container::Locked)
-      c.update_attributes!(state: Container::Running,
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running,
                            output: output_pdh,
                            log: log_pdh)
       c
@@ -310,7 +319,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     act_as_system_user do
       Collection.where(portable_data_hash: output_pdh).delete_all
       Collection.where(portable_data_hash: log_pdh).delete_all
-      c.update_attributes!(state: Container::Complete)
+      c.update!(state: Container::Complete)
     end
 
     cr.reload
@@ -328,8 +337,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
-      c.update_attributes!(state: Container::Locked)
-      c.update_attributes!(state: Container::Running)
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
       c
     end
 
@@ -389,14 +398,15 @@ class ContainerRequestTest < ActiveSupport::TestCase
     ]
     parents = toplevel_crs.map(&findctr)
 
-    children = parents.map do |parent|
+    children_crs = parents.map do |parent|
       lock_and_run(parent)
       with_container_auth(parent) do
         create_minimal_req!(state: "Committed",
                             priority: 1,
                             environment: {"child" => parent.environment["workflow"]})
       end
-    end.map(&findctr)
+    end
+    children = children_crs.map(&findctr)
 
     grandchildren = children.reverse.map do |child|
       lock_and_run(child)
@@ -448,7 +458,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     # increasing priority of the most recent toplevel container should
     # reprioritize all of its descendants (including the shared
     # grandchild) above everything else.
-    toplevel_crs[2].update_attributes!(priority: 72)
+    toplevel_crs[2].update!(priority: 72)
     (parents + children + grandchildren + [shared_grandchild]).map(&:reload)
     assert_operator shared_grandchild.priority, :>, grandchildren[0].priority
     assert_operator shared_grandchild.priority, :>, children[0].priority
@@ -461,6 +471,36 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_operator shared_grandchild.priority, :<=, grandchildren[2].priority
     assert_operator shared_grandchild.priority, :<=, children[2].priority
     assert_operator shared_grandchild.priority, :<=, parents[2].priority
+
+    # cancelling the most recent toplevel container should
+    # reprioritize all of its descendants (except the shared
+    # grandchild) to zero
+    toplevel_crs[2].update!(priority: 0)
+    (parents + children + grandchildren + [shared_grandchild]).map(&:reload)
+    assert_operator 0, :==, parents[2].priority
+    assert_operator 0, :==, children[2].priority
+    assert_operator 0, :==, grandchildren[2].priority
+    assert_operator shared_grandchild.priority, :==, grandchildren[0].priority
+
+    # cancel a child request, the parent should be > 0 but
+    # the child and grandchild go to 0.
+    children_crs[1].update!(priority: 0)
+    (parents + children + grandchildren + [shared_grandchild]).map(&:reload)
+    assert_operator 0, :<, parents[1].priority
+    assert_operator parents[0].priority, :>, parents[1].priority
+    assert_operator 0, :==, children[1].priority
+    assert_operator 0, :==, grandchildren[1].priority
+    assert_operator shared_grandchild.priority, :==, grandchildren[0].priority
+
+    # update the parent, it should get a higher priority but the children and
+    # grandchildren should remain at 0
+    toplevel_crs[1].update!(priority: 6)
+    (parents + children + grandchildren + [shared_grandchild]).map(&:reload)
+    assert_operator 0, :<, parents[1].priority
+    assert_operator parents[0].priority, :<, parents[1].priority
+    assert_operator 0, :==, children[1].priority
+    assert_operator 0, :==, grandchildren[1].priority
+    assert_operator shared_grandchild.priority, :==, grandchildren[0].priority
   end
 
   [
@@ -469,13 +509,49 @@ class ContainerRequestTest < ActiveSupport::TestCase
   ].each do |token, expected, expected_priority|
     test "create as #{token} and expect requesting_container_uuid to be #{expected}" do
       set_user_from_auth token
-      cr = ContainerRequest.create(container_image: "img", output_path: "/tmp", command: ["echo", "foo"])
+      cr = create_minimal_req!
       assert_not_nil cr.uuid, 'uuid should be set for newly created container_request'
       assert_equal expected, cr.requesting_container_uuid
       assert_equal expected_priority, cr.priority
     end
   end
 
+  [
+    [:admin, 0, "output"],
+    [:admin, 19, "output"],
+    [:admin, nil, "output"],
+    [:running_container_auth, 0, "intermediate"],
+    [:running_container_auth, 29, "intermediate"],
+    [:running_container_auth, nil, "intermediate"],
+  ].each do |token, exit_code, expect_output_type|
+    test "container with exit_code #{exit_code} has collection types set with output type #{expect_output_type}" do
+      final_state = if exit_code.nil?
+                      Container::Cancelled
+                    else
+                      Container::Complete
+                    end
+      set_user_from_auth token
+      request = create_minimal_req!(
+        container_count_max: 1,
+        priority: 500,
+        state: ContainerRequest::Committed,
+      )
+      run_container(request, final_state: final_state, exit_code: exit_code)
+      request.reload
+      assert_equal(ContainerRequest::Final, request.state)
+
+      output = Collection.find_by_uuid(request.output_uuid)
+      assert_not_nil(output)
+      assert_equal(request.uuid, output.properties["container_request"])
+      assert_equal(expect_output_type, output.properties["type"])
+
+      log = Collection.find_by_uuid(request.log_uuid)
+      assert_not_nil(log)
+      assert_equal(request.uuid, log.properties["container_request"])
+      assert_equal("log", log.properties["type"])
+    end
+  end
+
   test "create as container_runtime_token and expect requesting_container_uuid to be zzzzz-dz642-20isqbkl8xwnsao" do
     set_user_from_auth :container_runtime_token
     Thread.current[:token] = "#{Thread.current[:token]}/zzzzz-dz642-20isqbkl8xwnsao"
@@ -733,7 +809,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
       #   should be assigned.
       # * When use_existing is false, a different container should be assigned.
       # * When env1 and env2 are different, a different container should be assigned.
-      cr2.update_attributes!({state: ContainerRequest::Committed})
+      cr2.update!({state: ContainerRequest::Committed})
       assert_equal (cr2.use_existing == true and (env1 == env2)),
                    (cr1.container_uuid == cr2.container_uuid)
     end
@@ -754,8 +830,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
-      c.update_attributes!(state: Container::Locked)
-      c.update_attributes!(state: Container::Running)
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
       c
     end
 
@@ -767,7 +843,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
     prev_container_uuid = cr.container_uuid
 
     act_as_system_user do
-      c.update_attributes!(state: Container::Cancelled)
+      c.update!(cost: 0.5, subrequests_cost: 1.25)
+      c.update!(state: Container::Cancelled)
     end
 
     cr.reload
@@ -779,7 +856,10 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
-      c.update_attributes!(state: Container::Cancelled)
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
+      c.update!(cost: 0.125)
+      c.update!(state: Container::Cancelled)
       c
     end
 
@@ -788,6 +868,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal "Final", cr.state
     assert_equal prev_container_uuid, cr.container_uuid
     assert_not_equal cr2.container_uuid, cr.container_uuid
+    assert_equal 1.875, cr.cumulative_cost
   end
 
   test "Retry on container cancelled with runtime_token" do
@@ -801,8 +882,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
       assert_equal spec.token, c.runtime_token
-      c.update_attributes!(state: Container::Locked)
-      c.update_attributes!(state: Container::Running)
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
       c
     end
 
@@ -812,7 +893,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     prev_container_uuid = cr.container_uuid
 
     act_as_system_user do
-      c.update_attributes!(state: Container::Cancelled)
+      c.update!(state: Container::Cancelled)
     end
 
     cr.reload
@@ -823,7 +904,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
       assert_equal spec.token, c.runtime_token
-      c.update_attributes!(state: Container::Cancelled)
+      c.update!(state: Container::Cancelled)
       c
     end
 
@@ -839,8 +920,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
-      c.update_attributes!(state: Container::Locked)
-      c.update_attributes!(state: Container::Running)
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
       c
     end
 
@@ -855,7 +936,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
         logc = Collection.new(manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n")
         logc.save!
         c = Container.find_by_uuid(cr.container_uuid)
-        c.update_attributes!(state: Container::Cancelled, log: logc.portable_data_hash)
+        c.update!(state: Container::Cancelled, log: logc.portable_data_hash)
         c
       end
     end
@@ -873,6 +954,174 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
   end
 
+  test "Retry sub-request on error" do
+    set_user_from_auth :active
+    cr1 = create_minimal_req!(priority: 1, state: "Committed", container_count_max: 2, command: ["echo", "foo1"])
+    c1 = Container.find_by_uuid(cr1.container_uuid)
+    act_as_system_user do
+      c1.update!(state: Container::Locked)
+      c1.update!(state: Container::Running)
+    end
+
+    cr2 = with_container_auth(c1) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 2, command: ["echo", "foo2"])
+    end
+    c2 = Container.find_by_uuid(cr2.container_uuid)
+    act_as_system_user do
+      c2.update!(state: Container::Locked)
+      c2.update!(state: Container::Running)
+    end
+
+    cr3 = with_container_auth(c2) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 2, command: ["echo", "foo3"])
+    end
+    c3 = Container.find_by_uuid(cr3.container_uuid)
+
+    act_as_system_user do
+      c3.update!(state: Container::Locked)
+      c3.update!(state: Container::Running)
+    end
+
+    # All the containers are in running state
+
+    c3.reload
+    cr3.reload
+
+    # c3 still running
+    assert_equal 'Running', c3.state
+    assert_equal 1, cr3.container_count
+    assert_equal 'Committed', cr3.state
+
+    # c3 goes to cancelled state
+    act_as_system_user do
+      c3.state = "Cancelled"
+      c3.save!
+    end
+
+    cr3.reload
+
+    # Because the parent request is still live, it should
+    # be retried.
+    assert_equal 2, cr3.container_count
+    assert_equal 'Committed', cr3.state
+  end
+
+  test "Do not retry sub-request when process tree is cancelled" do
+    set_user_from_auth :active
+    cr1 = create_minimal_req!(priority: 1, state: "Committed", container_count_max: 2, command: ["echo", "foo1"])
+    c1 = Container.find_by_uuid(cr1.container_uuid)
+    act_as_system_user do
+      c1.update!(state: Container::Locked)
+      c1.update!(state: Container::Running)
+    end
+
+    cr2 = with_container_auth(c1) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 2, command: ["echo", "foo2"])
+    end
+    c2 = Container.find_by_uuid(cr2.container_uuid)
+    act_as_system_user do
+      c2.update!(state: Container::Locked)
+      c2.update!(state: Container::Running)
+    end
+
+    cr3 = with_container_auth(c2) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 2, command: ["echo", "foo3"])
+    end
+    c3 = Container.find_by_uuid(cr3.container_uuid)
+
+    act_as_system_user do
+      c3.update!(state: Container::Locked)
+      c3.update!(state: Container::Running)
+    end
+
+    # All the containers are in running state
+
+    # Now cancel the toplevel container request
+    act_as_system_user do
+      cr1.priority = 0
+      cr1.save!
+    end
+
+    c3.reload
+    cr3.reload
+
+    # c3 still running
+    assert_equal 'Running', c3.state
+    assert_equal 1, cr3.container_count
+    assert_equal 'Committed', cr3.state
+
+    # c3 goes to cancelled state
+    act_as_system_user do
+      assert_equal 0, c3.priority
+      c3.state = "Cancelled"
+      c3.save!
+    end
+
+    cr3.reload
+
+    # Because the parent process was cancelled, it _should not_ be
+    # retried.
+    assert_equal 1, cr3.container_count
+    assert_equal 'Final', cr3.state
+  end
+
+  test "Retry process tree on error" do
+    set_user_from_auth :active
+    cr1 = create_minimal_req!(priority: 1, state: "Committed", container_count_max: 2, command: ["echo", "foo1"])
+    c1 = Container.find_by_uuid(cr1.container_uuid)
+    act_as_system_user do
+      c1.update!(state: Container::Locked)
+      c1.update!(state: Container::Running)
+    end
+
+    cr2 = with_container_auth(c1) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 2, command: ["echo", "foo2"])
+    end
+    c2 = Container.find_by_uuid(cr2.container_uuid)
+    act_as_system_user do
+      c2.update!(state: Container::Locked)
+      c2.update!(state: Container::Running)
+    end
+
+    cr3 = with_container_auth(c2) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 2, command: ["echo", "foo3"])
+    end
+    c3 = Container.find_by_uuid(cr3.container_uuid)
+
+    act_as_system_user do
+      c3.update!(state: Container::Locked)
+      c3.update!(state: Container::Running)
+    end
+
+    # All the containers are in running state
+
+    c1.reload
+
+    # c1 goes to cancelled state
+    act_as_system_user do
+      c1.state = "Cancelled"
+      c1.save!
+    end
+
+    cr1.reload
+    cr2.reload
+    cr3.reload
+
+    # Because the root request is still live, it should be retried.
+    # Assumes the root is something like arvados-cwl-runner where
+    # container reuse enables it to more or less pick up where it left
+    # off.
+    assert_equal 2, cr1.container_count
+    assert_equal 'Committed', cr1.state
+
+    # These keep running.
+    assert_equal 1, cr2.container_count
+    assert_equal 'Committed', cr2.state
+
+    assert_equal 1, cr3.container_count
+    assert_equal 'Committed', cr3.state
+  end
+
   test "Output collection name setting using output_name with name collision resolution" do
     set_user_from_auth :active
     output_name = 'unimaginative name'
@@ -886,13 +1135,13 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal ContainerRequest::Final, cr.state
     output_coll = Collection.find_by_uuid(cr.output_uuid)
     # Make sure the resulting output collection name include the original name
-    # plus the date
+    # plus the last 15 characters of uuid
     assert_not_equal output_name, output_coll.name,
                      "more than one collection with the same owner and name"
     assert output_coll.name.include?(output_name),
            "New name should include original name"
-    assert_match /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z/, output_coll.name,
-                 "New name should include ISO8601 date"
+    assert_match /#{output_coll.uuid[-15..-1]}/, output_coll.name,
+                 "New name should include last 15 characters of uuid"
   end
 
   [[0, :check_output_ttl_0],
@@ -933,17 +1182,17 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_in_delta(delete, now + year, 10)
   end
 
-  def run_container(cr)
+  def run_container(cr, final_state: Container::Complete, exit_code: 0)
     act_as_system_user do
       logc = Collection.new(owner_uuid: system_user_uuid,
                             manifest_text: ". ef772b2f28e2c8ca84de45466ed19ee9+7815 0:0:arv-mount.txt\n")
       logc.save!
 
       c = Container.find_by_uuid(cr.container_uuid)
-      c.update_attributes!(state: Container::Locked)
-      c.update_attributes!(state: Container::Running)
-      c.update_attributes!(state: Container::Complete,
-                           exit_code: 0,
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
+      c.update!(state: final_state,
+                           exit_code: exit_code,
                            output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
                            log: logc.portable_data_hash)
       logc.destroy
@@ -966,7 +1215,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     cr3 = create_minimal_req!(priority: 1, state: ContainerRequest::Uncommitted)
     assert_equal ContainerRequest::Uncommitted, cr3.state
-    cr3.update_attributes!(state: ContainerRequest::Committed)
+    cr3.update!(state: ContainerRequest::Committed)
     assert_equal cr.container_uuid, cr3.container_uuid
     assert_equal ContainerRequest::Final, cr3.state
   end
@@ -1062,7 +1311,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
       # Even though preemptible is not allowed, we should be able to
       # commit a CR that was created earlier when preemptible was the
       # default.
-      commit_later.update_attributes!(priority: 1, state: "Committed")
+      commit_later.update!(priority: 1, state: "Committed")
       expect[false].push commit_later
     end
 
@@ -1078,7 +1327,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
       # Cancelling the parent used to fail while updating the child
       # containers' priority, because the child containers' unchanged
       # preemptible fields caused validation to fail.
-      parent.update_attributes!(state: 'Cancelled')
+      parent.update!(state: 'Cancelled')
 
       [false, true].each do |pflag|
         expect[pflag].each do |cr|
@@ -1126,7 +1375,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
     end
   end
 
-  test "Having preemptible_instances=true create a committed child container request and verify the scheduling parameter of its container" do
+  test "AlwaysUsePreemptibleInstances makes child containers preemptible" do
+    Rails.configuration.Containers.AlwaysUsePreemptibleInstances = true
     common_attrs = {cwd: "test",
                     priority: 1,
                     command: ["echo", "hello"],
@@ -1204,7 +1454,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
         when 'Final'
           act_as_system_user do
             Container.find_by_uuid(cr.container_uuid).
-              update_attributes!(state: Container::Cancelled)
+              update!(state: Container::Cancelled)
           end
           cr.reload
         else
@@ -1212,10 +1462,10 @@ class ContainerRequestTest < ActiveSupport::TestCase
         end
         assert_equal state, cr.state
         if permitted
-          assert cr.update_attributes!(updates)
+          assert cr.update!(updates)
         else
           assert_raises(ActiveRecord::RecordInvalid) do
-            cr.update_attributes!(updates)
+            cr.update!(updates)
           end
         end
       end
@@ -1233,8 +1483,41 @@ class ContainerRequestTest < ActiveSupport::TestCase
       cr.destroy
 
       # the cr's container now has priority of 0
+      c.reload
+      assert_equal 0, c.priority
+    end
+  end
+
+  test "trash the project containing a container_request and check its container's priority" do
+    act_as_user users(:active) do
+      cr = ContainerRequest.find_by_uuid container_requests(:running_to_be_deleted).uuid
+
+      # initially the cr's container has priority > 0
       c = Container.find_by_uuid(cr.container_uuid)
+      assert_equal 1, c.priority
+
+      prj = Group.find_by_uuid cr.owner_uuid
+      prj.update!(trash_at: db_current_time)
+
+      # the cr's container now has priority of 0
+      c.reload
       assert_equal 0, c.priority
+
+      assert_equal c.state, 'Running'
+      assert_equal cr.state, 'Committed'
+
+      # mark the container as cancelled, this should cause the
+      # container request to go to final state and run the finalize
+      # function
+      act_as_system_user do
+        c.update!(state: 'Cancelled', log: 'fa7aeb5140e2848d39b416daeef4ffc5+45')
+      end
+      c.reload
+      cr.reload
+
+      assert_equal c.state, 'Cancelled'
+      assert_equal cr.state, 'Final'
+      assert_equal nil, cr.log_uuid
     end
   end
 
@@ -1336,7 +1619,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     sm = {'/secret/foo' => {'kind' => 'text', 'content' => secret_string}}
     set_user_from_auth :active
     cr = create_minimal_req!
-    assert_equal false, cr.update_attributes(state: "Committed",
+    assert_equal false, cr.update(state: "Committed",
                                              priority: 1,
                                              mounts: cr.mounts.merge(sm),
                                              secret_mounts: sm)
@@ -1356,7 +1639,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_not_nil ApiClientAuthorization.find_by_uuid(spec.uuid)
 
     act_as_system_user do
-      c.update_attributes!(state: Container::Complete,
+      c.update!(state: Container::Complete,
                            exit_code: 0,
                            output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
                            log: 'fa7aeb5140e2848d39b416daeef4ffc5+45')
@@ -1435,7 +1718,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_nil cr2.container_uuid
 
     # Update cr2 to commited state, check for reuse, then run it
-    cr2.update_attributes!({state: ContainerRequest::Committed})
+    cr2.update!({state: ContainerRequest::Committed})
     assert_equal cr1.container_uuid, cr2.container_uuid
 
     cr2.reload
@@ -1447,4 +1730,105 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal ["foo_storage_class"], output1.storage_classes_desired
     assert_equal ["bar_storage_class"], output2.storage_classes_desired
   end
+
+  [
+    [{},               {},           {"type": "output"}],
+    [{"a1": "b1"},     {},           {"type": "output", "a1": "b1"}],
+    [{},               {"a1": "b1"}, {"type": "output", "a1": "b1"}],
+    [{"a1": "b1"},     {"a1": "c1"}, {"type": "output", "a1": "b1"}],
+    [{"a1": "b1"},     {"a2": "c2"}, {"type": "output", "a1": "b1", "a2": "c2"}],
+    [{"type": "blah"}, {},           {"type": "blah"}],
+  ].each do |cr_prop, container_prop, expect_prop|
+    test "setting output_properties #{cr_prop} #{container_prop} on current container" do
+      act_as_user users(:active) do
+        cr = create_minimal_req!(priority: 1,
+                                 state: ContainerRequest::Committed,
+                                 output_name: 'foo',
+                                 output_properties: cr_prop)
+
+        act_as_system_user do
+          logc = Collection.new(owner_uuid: system_user_uuid,
+                                manifest_text: ". ef772b2f28e2c8ca84de45466ed19ee9+7815 0:0:arv-mount.txt\n")
+          logc.save!
+
+          c = Container.find_by_uuid(cr.container_uuid)
+          c.update!(state: Container::Locked)
+          c.update!(state: Container::Running)
+
+          c.update!(output_properties: container_prop)
+
+          c.update!(state: Container::Complete,
+                               exit_code: 0,
+                               output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
+                               log: logc.portable_data_hash)
+          logc.destroy
+        end
+
+        cr.reload
+        expect_prop["container_request"] = cr.uuid
+        output = Collection.find_by_uuid(cr.output_uuid)
+        assert_equal expect_prop.symbolize_keys, output.properties.symbolize_keys
+      end
+    end
+  end
+
+  test "Cumulative cost includes retried attempts but not reused containers" do
+    set_user_from_auth :active
+    cr = create_minimal_req!(priority: 5, state: "Committed", container_count_max: 3)
+    c = Container.find_by_uuid cr.container_uuid
+    act_as_system_user do
+      c.update!(state: Container::Locked)
+      c.update!(state: Container::Running)
+      c.update!(state: Container::Cancelled, cost: 3)
+    end
+    cr.reload
+    assert_equal 3, cr.cumulative_cost
+
+    c = Container.find_by_uuid cr.container_uuid
+    lock_and_run c
+    c.reload
+    assert_equal 0, c.subrequests_cost
+
+    # cr2 is a child/subrequest
+    cr2 = with_container_auth(c) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 1, command: ["echo", "foo2"])
+    end
+    assert_equal c.uuid, cr2.requesting_container_uuid
+    c2 = Container.find_by_uuid cr2.container_uuid
+    act_as_system_user do
+      c2.update!(state: Container::Locked)
+      c2.update!(state: Container::Running)
+      logc = Collection.new(owner_uuid: system_user_uuid,
+                            manifest_text: ". ef772b2f28e2c8ca84de45466ed19ee9+7815 0:0:arv-mount.txt\n")
+      logc.save!
+      c2.update!(state: Container::Complete,
+                            exit_code: 0,
+                            output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
+                            log: logc.portable_data_hash,
+                            cost: 7)
+    end
+    c.reload
+    assert_equal 7, c.subrequests_cost
+
+    # cr3 is an identical child/subrequest, will reuse c2
+    cr3 = with_container_auth(c) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 1, command: ["echo", "foo2"])
+    end
+    assert_equal c.uuid, cr3.requesting_container_uuid
+    c3 = Container.find_by_uuid cr3.container_uuid
+    assert_equal c2.uuid, c3.uuid
+    assert_equal Container::Complete, c3.state
+    c.reload
+    assert_equal 7, c.subrequests_cost
+
+    act_as_system_user do
+      c.update!(state: Container::Complete, exit_code: 0, cost: 9)
+    end
+
+    c.reload
+    assert_equal 7, c.subrequests_cost
+    cr.reload
+    assert_equal 3+7+9, cr.cumulative_cost
+  end
+
 end