X-Git-Url: https://git.arvados.org/arvados.git/blobdiff_plain/dee79c4cf4eb34d80b925168e76d1114dfc02c2a..46f3bff06569f06ce84799635ad25727cfd095b5:/services/api/app/models/container.rb?ds=inline

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 95691687e6..2bbdd0a07f 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -89,7 +89,8 @@ class Container < ArvadosModel
     nil => [Queued],
     Queued => [Locked, Cancelled],
     Locked => [Queued, Running, Cancelled],
-    Running => [Complete, Cancelled]
+    Running => [Complete, Cancelled],
+    Complete => [Cancelled]
   }
 
   def self.limit_index_columns_read
@@ -497,7 +498,7 @@ class Container < ArvadosModel
       return false
     end
 
-    if self.state == Running &&
+    if self.state_was == Running &&
        !current_api_client_authorization.nil? &&
        (current_api_client_authorization.uuid == self.auth_uuid ||
         current_api_client_authorization.token == self.runtime_token)
@@ -505,6 +506,8 @@ class Container < ArvadosModel
       # change priority or log.
       permitted.push *final_attrs
       permitted = permitted - [:log, :priority]
+    elsif !current_user.andand.is_admin
+      raise PermissionDeniedError
     elsif self.locked_by_uuid && self.locked_by_uuid != current_api_client_authorization.andand.uuid
       # When locked, progress fields cannot be updated by the wrong
       # dispatcher, even though it has admin privileges.
@@ -645,64 +648,76 @@ class Container < ArvadosModel
     # This container is finished so finalize any associated container requests
     # that are associated with this container.
     if self.state_changed? and self.final?
-      act_as_system_user do
-
-        if self.state == Cancelled
-          retryable_requests = ContainerRequest.where("container_uuid = ? and priority > 0 and state = 'Committed' and container_count < container_count_max", uuid)
-        else
-          retryable_requests = []
-        end
+      # These get wiped out by with_lock (which reloads the record),
+      # so record them now in case we need to schedule a retry.
+      prev_secret_mounts = self.secret_mounts_was
+      prev_runtime_token = self.runtime_token_was
+
+      # Need to take a lock on the container to ensure that any
+      # concurrent container requests that might try to reuse this
+      # container will block until the container completion
+      # transaction finishes.  This ensure that concurrent container
+      # requests that try to reuse this container are finalized (on
+      # Complete) or don't reuse it (on Cancelled).
+      self.with_lock do
+        act_as_system_user do
+          if self.state == Cancelled
+            retryable_requests = ContainerRequest.where("container_uuid = ? and priority > 0 and state = 'Committed' and container_count < container_count_max", uuid)
+          else
+            retryable_requests = []
+          end
 
-        if retryable_requests.any?
-          c_attrs = {
-            command: self.command,
-            cwd: self.cwd,
-            environment: self.environment,
-            output_path: self.output_path,
-            container_image: self.container_image,
-            mounts: self.mounts,
-            runtime_constraints: self.runtime_constraints,
-            scheduling_parameters: self.scheduling_parameters,
-            secret_mounts: self.secret_mounts_was,
-            runtime_token: self.runtime_token_was,
-            runtime_user_uuid: self.runtime_user_uuid,
-            runtime_auth_scopes: self.runtime_auth_scopes
-          }
-          c = Container.create! c_attrs
-          retryable_requests.each do |cr|
-            cr.with_lock do
-              leave_modified_by_user_alone do
-                # Use row locking because this increments container_count
-                cr.container_uuid = c.uuid
-                cr.save!
+          if retryable_requests.any?
+            c_attrs = {
+              command: self.command,
+              cwd: self.cwd,
+              environment: self.environment,
+              output_path: self.output_path,
+              container_image: self.container_image,
+              mounts: self.mounts,
+              runtime_constraints: self.runtime_constraints,
+              scheduling_parameters: self.scheduling_parameters,
+              secret_mounts: prev_secret_mounts,
+              runtime_token: prev_runtime_token,
+              runtime_user_uuid: self.runtime_user_uuid,
+              runtime_auth_scopes: self.runtime_auth_scopes
+            }
+            c = Container.create! c_attrs
+            retryable_requests.each do |cr|
+              cr.with_lock do
+                leave_modified_by_user_alone do
+                  # Use row locking because this increments container_count
+                  cr.container_uuid = c.uuid
+                  cr.save!
+                end
               end
             end
           end
-        end
 
-        # Notify container requests associated with this container
-        ContainerRequest.where(container_uuid: uuid,
-                               state: ContainerRequest::Committed).each do |cr|
-          leave_modified_by_user_alone do
-            cr.finalize!
+          # Notify container requests associated with this container
+          ContainerRequest.where(container_uuid: uuid,
+                                 state: ContainerRequest::Committed).each do |cr|
+            leave_modified_by_user_alone do
+              cr.finalize!
+            end
           end
-        end
 
-        # Cancel outstanding container requests made by this container.
-        ContainerRequest.
-          includes(:container).
-          where(requesting_container_uuid: uuid,
-                state: ContainerRequest::Committed).each do |cr|
-          leave_modified_by_user_alone do
-            cr.update_attributes!(priority: 0)
-            cr.container.reload
-            if cr.container.state == Container::Queued || cr.container.state == Container::Locked
-              # If the child container hasn't started yet, finalize the
-              # child CR now instead of leaving it "on hold", i.e.,
-              # Queued with priority 0.  (OTOH, if the child is already
-              # running, leave it alone so it can get cancelled the
-              # usual way, get a copy of the log collection, etc.)
-              cr.update_attributes!(state: ContainerRequest::Final)
+          # Cancel outstanding container requests made by this container.
+          ContainerRequest.
+            includes(:container).
+            where(requesting_container_uuid: uuid,
+                  state: ContainerRequest::Committed).each do |cr|
+            leave_modified_by_user_alone do
+              cr.update_attributes!(priority: 0)
+              cr.container.reload
+              if cr.container.state == Container::Queued || cr.container.state == Container::Locked
+                # If the child container hasn't started yet, finalize the
+                # child CR now instead of leaving it "on hold", i.e.,
+                # Queued with priority 0.  (OTOH, if the child is already
+                # running, leave it alone so it can get cancelled the
+                # usual way, get a copy of the log collection, etc.)
+                cr.update_attributes!(state: ContainerRequest::Final)
+              end
             end
           end
         end