[ARVADOS] created: 1.1.4-351-g7e6ac5a

[Git user]

        at  7e6ac5a4967614cbe59ed5c0ec41c8be4d4cff4d (commit)


commit 7e6ac5a4967614cbe59ed5c0ec41c8be4d4cff4d
Author: Tom Clegg <tclegg at veritasgenetics.com>
Date:   Tue Jun 5 10:56:22 2018 -0400

    13164: Remove locking. Clean up after races in background instead.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg at veritasgenetics.com>

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 1dbdb57..7ec9845 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -5,6 +5,7 @@
 require 'log_reuse_info'
 require 'whitelist_update'
 require 'safe_json'
+require 'update_priority'
 
 class Container < ArvadosModel
   include ArvadosModelUpdates
@@ -37,6 +38,7 @@ class Container < ArvadosModel
   before_save :scrub_secret_mounts
   after_save :handle_completed
   after_save :propagate_priority
+  after_commit { UpdatePriority.run_update_thread }
 
   has_many :container_requests, :foreign_key => :container_uuid, :class_name => 'ContainerRequest', :primary_key => :uuid
   belongs_to :auth, :class_name => 'ApiClientAuthorization', :foreign_key => :auth_uuid, :primary_key => :uuid
@@ -126,7 +128,6 @@ class Container < ArvadosModel
       # Update the priority of child container requests to match new
       # priority of the parent container (ignoring requests with no
       # container assigned, because their priority doesn't matter).
-      ActiveRecord::Base.connection.execute('LOCK container_requests, containers IN EXCLUSIVE MODE')
       ContainerRequest.
         where(requesting_container_uuid: self.uuid,
               state: ContainerRequest::Committed).
@@ -316,10 +317,6 @@ class Container < ArvadosModel
     # (because state might have changed while acquiring the lock).
     check_lock_fail
     transaction do
-      # Locking involves assigning auth_uuid, which involves looking
-      # up container requests, so we must lock both tables in the
-      # proper order to avoid deadlock.
-      ActiveRecord::Base.connection.execute('LOCK container_requests, containers IN EXCLUSIVE MODE')
       reload
       check_lock_fail
       update_attributes!(state: Locked)
@@ -542,7 +539,6 @@ class Container < ArvadosModel
     if self.state_changed? and self.final?
       act_as_system_user do
 
-        ActiveRecord::Base.connection.execute('LOCK container_requests, containers IN EXCLUSIVE MODE')
         if self.state == Cancelled
           retryable_requests = ContainerRequest.where("container_uuid = ? and priority > 0 and state = 'Committed' and container_count < container_count_max", uuid)
         else
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index ac4415b..b4d1a41 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -286,7 +286,6 @@ class ContainerRequest < ArvadosModel
   def update_priority
     return unless state_changed? || priority_changed? || container_uuid_changed?
     act_as_system_user do
-      ActiveRecord::Base.connection.execute('LOCK container_requests, containers IN EXCLUSIVE MODE')
       Container.
         where('uuid in (?)', [self.container_uuid_was, self.container_uuid].compact).
         map(&:update_priority!)
@@ -299,7 +298,6 @@ class ContainerRequest < ArvadosModel
 
   def set_requesting_container_uuid
     return if !current_api_client_authorization
-    ActiveRecord::Base.connection.execute('LOCK container_requests, containers IN EXCLUSIVE MODE')
     if (c = Container.where('auth_uuid=?', current_api_client_authorization.uuid).select([:uuid, :priority]).first)
       self.requesting_container_uuid = c.uuid
       self.priority = c.priority>0 ? 1 : 0
diff --git a/services/api/lib/update_priority.rb b/services/api/lib/update_priority.rb
new file mode 100644
index 0000000..724d2b2
--- /dev/null
+++ b/services/api/lib/update_priority.rb
@@ -0,0 +1,38 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+module UpdatePriority
+  # Clean up after races: if container priority>0 but there are no
+  # committed container requests for it, reset priority to 0.
+  def self.update_priority
+    if !File.owned?(Rails.root.join('tmp'))
+      Rails.logger.warn("UpdatePriority: not owner of #{Rails.root}/tmp, skipping")
+      return
+    end
+    lockfile = Rails.root.join('tmp', 'update_priority.lock')
+    File.open(lockfile, File::RDWR|File::CREAT, 0600) do |f|
+      return unless f.flock(File::LOCK_NB|File::LOCK_EX)
+      ActiveRecord::Base.connection.execute("UPDATE containers AS c SET priority=0 WHERE state='Queued' AND priority>0 AND uuid NOT IN (SELECT container_uuid FROM container_requests WHERE priority>0);")
+    end
+  end
+
+  def self.run_update_thread
+    need = false
+    Rails.cache.fetch('UpdatePriority', expires_in: 5.seconds) do
+      need = true
+    end
+    return if !need
+
+    Thread.new do
+      Thread.current.abort_on_exception = false
+      begin
+        update_priority
+      rescue => e
+        Rails.logger.error "#{e.class}: #{e}\n#{e.backtrace.join("\n\t")}"
+      ensure
+        ActiveRecord::Base.connection.close
+      end
+    end
+  end
+end

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


hooks/post-receive
--