From: Peter Amstutz <peter.amstutz@curoverse.com>
Date: Wed, 9 Dec 2015 20:48:37 +0000 (-0500)
Subject: 6429: Complete/Cancelled containers finalize associated container requests;
X-Git-Tag: 1.1.0~1198^2~5
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/d4939c581f629a19220bc5b469fed98462b2b7eb

6429: Complete/Cancelled containers finalize associated container requests;
container requests update priority for associated containers.
---

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 15bbbe017d..79c9868189 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -17,7 +17,6 @@ class Container < ArvadosModel
   validate :validate_state_change
   validate :validate_change
   after_save :request_finalize
-  after_save :process_tree_priority
 
   has_many :container_requests, :foreign_key => :container_uuid, :class_name => 'ContainerRequest', :primary_key => :uuid
 
@@ -151,22 +150,18 @@ class Container < ArvadosModel
     # that are associated with this container.
     if self.state_changed? and [Complete, Cancelled].include? self.state
       act_as_system_user do
-        # Note using update_all skips model validation and callbacks.
-        ContainerRequest.update_all({:state => ContainerRequest::Final}, ['container_uuid=?', uuid])
-      end
-    end
-  end
+        # Try to close container requests associated with this container
+        ContainerRequest.where(container_uuid: uuid,
+                               :state => ContainerRequest::Committed).each do |cr|
+          cr.state = ContainerRequest::Final
+          cr.save
+        end
 
-  def process_tree_priority
-    # This container is cancelled so cancel any container requests made by this
-    # container.
-    if self.priority_changed? and self.priority == 0
-      # This could propagate any parent priority to the children (not just
-      # priority 0)
-      act_as_system_user do
-        ContainerRequest.where(requesting_container_uuid: uuid).each do |cr|
-          cr.priority = self.priority
-          cr.save!
+        # Try to close any outstanding container requests made by this container.
+        ContainerRequest.where(requesting_container_uuid: uuid,
+                               :state => ContainerRequest::Committed).each do |cr|
+          cr.state = ContainerRequest::Final
+          cr.save
         end
       end
     end
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 1011f6fd75..4e76a0e08a 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -145,12 +145,16 @@ class ContainerRequest < ArvadosModel
   end
 
   def update_priority
-    if self.state == Committed and (self.state_changed? or
-                                    self.priority_changed? or
-                                    self.container_uuid_changed?)
-      c = Container.find_by_uuid self.container_uuid
-      act_as_system_user do
-        c.update_priority!
+    if [Committed, Final].include? self.state and (self.state_changed? or
+                                                   self.priority_changed? or
+                                                   self.container_uuid_changed?)
+      [self.container_uuid_was, self.container_uuid].each do |cuuid|
+        unless cuuid.nil?
+          c = Container.find_by_uuid cuuid
+          act_as_system_user do
+            c.update_priority!
+          end
+        end
       end
     end
   end
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 6d1a57699b..6470b785e2 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -199,7 +199,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
   end
 
-  test "Container cancel process tree" do
+  test "Container request finalize" do
     set_user_from_auth :active_trustedclient
     c = ContainerRequest.new
     c.state = "Committed"
@@ -210,31 +210,15 @@ class ContainerRequestTest < ActiveSupport::TestCase
     c.priority = 5
     c.save!
 
-    c2 = ContainerRequest.new
-    c2.state = "Committed"
-    c2.container_image = "img"
-    c2.command = ["foo", "bar"]
-    c2.output_path = "/tmp"
-    c2.cwd = "/tmp"
-    c2.priority = 10
-    c2.requesting_container_uuid = c.container_uuid
-    c2.save!
-
     t = Container.find_by_uuid c.container_uuid
     assert_equal 5, t.priority
 
-    t2 = Container.find_by_uuid c2.container_uuid
-    assert_equal 10, t2.priority
-
-    c.priority = 0
+    c.state = "Final"
     c.save!
 
     t.reload
     assert_equal 0, t.priority
 
-    t2.reload
-    assert_equal 0, t2.priority
-
   end