[ARVADOS] updated: 068b2c9dbe1e467119680cbab61c62a03ad4fafd

[Git user]

Summary of changes:
 services/api/app/models/container.rb             |  7 +++--
 services/api/app/models/container_request.rb     | 10 ++++--
 services/api/config/application.default.yml      |  7 +++++
 services/api/test/unit/container_request_test.rb | 40 ++++++++++++++++++++++--
 4 files changed, 57 insertions(+), 7 deletions(-)

       via  068b2c9dbe1e467119680cbab61c62a03ad4fafd (commit)
      from  49987769d924c1bc77cbdc9e9b182c3e2cc09b2d (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 068b2c9dbe1e467119680cbab61c62a03ad4fafd
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Tue Oct 4 15:56:55 2016 -0400

    8018: Fix state transition checks.  Add test of retry and fix other tests to
    not retry.

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 037712d..82eda47 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -298,8 +298,11 @@ class Container < ArvadosModel
                                 mounts: self.mounts,
                                 runtime_constraints: self.runtime_constraints)
           retryable_requests.each do |cr|
-            cr.container_uuid = c.uuid
-            cr.save!
+            cr.with_lock do
+              # Use row locking because this increments container_count
+              cr.container_uuid = c.uuid
+              cr.save
+            end
           end
         end
 
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 27401fc..206d94f 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -79,6 +79,7 @@ class ContainerRequest < ArvadosModel
     self.runtime_constraints ||= {}
     self.mounts ||= {}
     self.cwd ||= "."
+    self.container_count_max ||= Rails.configuration.container_count_max
   end
 
   # Create a new container (or find an existing one) to satisfy this
@@ -177,7 +178,12 @@ class ContainerRequest < ArvadosModel
       resolve
     end
     if self.container_uuid != self.container_uuid_was
-      self.container_count += 1
+      if self.container_count_changed?
+        errors.add :container_count, "cannot be updated directly."
+        return false
+      else
+        self.container_count += 1
+      end
     end
   end
 
@@ -216,7 +222,7 @@ class ContainerRequest < ArvadosModel
       end
 
       # Can update priority, container count, name and description
-      permitted.push :priority, :container_count_max, :container_uuid, :name, :description
+      permitted.push :priority, :container_count, :container_count_max, :container_uuid, :name, :description
 
       if self.state_changed?
         # Allow create-and-commit in a single operation.
diff --git a/services/api/config/application.default.yml b/services/api/config/application.default.yml
index 99d2a10..e3db027 100644
--- a/services/api/config/application.default.yml
+++ b/services/api/config/application.default.yml
@@ -375,6 +375,13 @@ common:
   # graph cache.  This feature is experimental!
   async_permissions_update: false
 
+  # Default value for container_count_max for container requests.  This is the
+  # number of times Arvados will create a new container to satisfy a container
+  # request.  If a container is cancelled it will retry a new container if
+  # container_count < container_count_max on any container requests associated
+  # with the cancelled container.
+  container_count_max: 3
+
 development:
   force_ssl: false
   cache_classes: false
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index e44084e..ff9e685 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -202,7 +202,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
   test "Request is finalized when its container is cancelled" do
     set_user_from_auth :active
-    cr = create_minimal_req!(priority: 1, state: "Committed")
+    cr = create_minimal_req!(priority: 1, state: "Committed", container_count_max: 1)
 
     act_as_system_user do
       Container.find_by_uuid(cr.container_uuid).
@@ -237,13 +237,13 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
   test "Container makes container request, then is cancelled" do
     set_user_from_auth :active
-    cr = create_minimal_req!(priority: 5, state: "Committed")
+    cr = create_minimal_req!(priority: 5, state: "Committed", container_count_max: 1)
 
     c = Container.find_by_uuid cr.container_uuid
     assert_equal 5, c.priority
 
     cr2 = create_minimal_req!
-    cr2.update_attributes!(priority: 10, state: "Committed", requesting_container_uuid: c.uuid, command: ["echo", "foo2"])
+    cr2.update_attributes!(priority: 10, state: "Committed", requesting_container_uuid: c.uuid, command: ["echo", "foo2"], container_count_max: 1)
     cr2.reload
 
     c2 = Container.find_by_uuid cr2.container_uuid
@@ -432,4 +432,38 @@ class ContainerRequestTest < ActiveSupport::TestCase
       create_minimal_req!(state: "Uncommitted", priority: 1, requesting_container_uuid: 'youcantdothat')
     end
   end
+
+  test "Retry on container cancelled" do
+    set_user_from_auth :active
+    cr = create_minimal_req!(priority: 1, state: "Committed", container_count_max: 2)
+
+    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
+    end
+
+    cr.reload
+    assert_equal "Committed", cr.state
+    old_container_uuid = cr.container_uuid
+
+    act_as_system_user do
+      c.update_attributes!(state: Container::Cancelled)
+    end
+
+    cr.reload
+    assert_equal "Committed", cr.state
+    assert_not_equal old_container_uuid, cr.container_uuid
+
+    c = act_as_system_user do
+      c = Container.find_by_uuid(cr.container_uuid)
+      c.update_attributes!(state: Container::Cancelled)
+      c
+    end
+
+    cr.reload
+    assert_equal "Final", cr.state
+  end
+
 end

-----------------------------------------------------------------------


hooks/post-receive
--