From: Lucas Di Pentima <ldipentima@veritasgenetics.com>
Date: Tue, 14 Aug 2018 14:25:10 +0000 (-0300)
Subject: 13773: Fix reuse query & add tests
X-Git-Tag: 1.3.0~111^2~17
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/fda792680a8fd21b1c80ea2a79b267381521935a?ds=sidebyside

13773: Fix reuse query & add tests

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima@veritasgenetics.com>
---

diff --git a/sdk/cwl/arvados_cwl/__init__.py b/sdk/cwl/arvados_cwl/__init__.py
index 8c3f0eadee..6edf00e7fe 100644
--- a/sdk/cwl/arvados_cwl/__init__.py
+++ b/sdk/cwl/arvados_cwl/__init__.py
@@ -133,7 +133,7 @@ class ArvCwlRunner(object):
             if arvargs.work_api is None:
                 raise Exception("No supported APIs")
             else:
-                raise Exception("Unsupported API '%s', expected one of %s" % (work_api, expected_api))
+                raise Exception("Unsupported API '%s', expected one of %s" % (arvargs.work_api, expected_api))
 
         if self.work_api == "jobs":
             logger.warn("""
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 380d4aafbe..0228eb2af6 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -280,7 +280,7 @@ class Container < ArvadosModel
     # Check for non-failing Running candidates and return the most likely to finish sooner.
     log_reuse_info { "checking for state=Running..." }
     running = candidates.where(state: Running).
-              where("NOT (runtime_status ? 'error')").
+              where("(runtime_status->'error') is null").
               order('progress desc, started_at asc').
               limit(1).first
     if running
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 83ab59b607..b1d35c55f0 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -131,6 +131,57 @@ class ContainerTest < ActiveSupport::TestCase
     end
   end
 
+  test "Container runtime_status updates" do
+    set_user_from_auth :active
+    attrs = {
+      environment: {},
+      mounts: {"BAR" => "FOO"},
+      output_path: "/tmp",
+      priority: 1,
+      runtime_constraints: {"vcpus" => 1, "ram" => 1}
+    }
+    c1, _ = minimal_new(attrs)
+    assert_equal c1.runtime_status, {}
+
+    assert_equal Container::Queued, c1.state
+    assert_raises ActiveRecord::RecordInvalid do
+      c1.update_attributes! runtime_status: {'error' => 'Oops!'}
+    end
+
+    set_user_from_auth :dispatch1
+
+    # Allow updates when state = Locked
+    c1.update_attributes! state: Container::Locked
+    c1.update_attributes! runtime_status: {'error' => 'Oops!'}
+    assert c1.runtime_status.key? 'error'
+
+    # Reset when transitioning from Locked to Queued
+    c1.update_attributes! state: Container::Queued
+    assert_equal c1.runtime_status, {}
+
+    # Allow updates when state = Running
+    c1.update_attributes! state: Container::Locked
+    c1.update_attributes! state: Container::Running
+    c1.update_attributes! runtime_status: {'error' => 'Oops!'}
+    assert c1.runtime_status.key? 'error'
+
+    # Don't allow updates on other states
+    c1.update_attributes! state: Container::Complete
+    assert_raises ActiveRecord::RecordInvalid do
+      c1.update_attributes! runtime_status: {'error' => 'Some other error'}
+    end
+
+    set_user_from_auth :active
+    c2, _ = minimal_new(attrs)
+    assert_equal c2.runtime_status, {}
+    set_user_from_auth :dispatch1
+    c2.update_attributes! state: Container::Locked
+    c2.update_attributes! state: Container::Running
+    c2.update_attributes! state: Container::Cancelled
+    assert_raises ActiveRecord::RecordInvalid do
+      c2.update_attributes! runtime_status: {'error' => 'Oops!'}
+    end
+  end
 
   test "Container serialized hash attributes sorted before save" do
     env = {"C" => "3", "B" => "2", "A" => "1"}
@@ -277,6 +328,31 @@ class ContainerTest < ActiveSupport::TestCase
     assert_equal reused.uuid, c_faster_started_second.uuid
   end
 
+  test "find_reusable method should select non-failing running container" do
+    set_user_from_auth :active
+    common_attrs = REUSABLE_COMMON_ATTRS.merge({environment: {"var" => "running2"}})
+    c_slower, _ = minimal_new(common_attrs.merge({use_existing: false}))
+    c_faster_started_first, _ = minimal_new(common_attrs.merge({use_existing: false}))
+    c_faster_started_second, _ = minimal_new(common_attrs.merge({use_existing: false}))
+    # Confirm the 3 container UUIDs are different.
+    assert_equal 3, [c_slower.uuid, c_faster_started_first.uuid, c_faster_started_second.uuid].uniq.length
+    set_user_from_auth :dispatch1
+    c_slower.update_attributes!({state: Container::Locked})
+    c_slower.update_attributes!({state: Container::Running,
+                                 progress: 0.1})
+    c_faster_started_first.update_attributes!({state: Container::Locked})
+    c_faster_started_first.update_attributes!({state: Container::Running,
+                                               progress: 0.15})
+    c_faster_started_second.update_attributes!({state: Container::Locked})
+    c_faster_started_second.update_attributes!({state: Container::Running,
+                                                runtime_status: {'error' => 'Something bad happened'},
+                                                progress: 0.2})
+    reused = Container.find_reusable(common_attrs)
+    assert_not_nil reused
+    # Selected the non-failing container even if it's the one with less progress done
+    assert_equal reused.uuid, c_faster_started_first.uuid
+  end
+
   test "find_reusable method should select locked container most likely to start sooner" do
     set_user_from_auth :active
     common_attrs = REUSABLE_COMMON_ATTRS.merge({environment: {"var" => "locked"}})