From: Tom Clegg <tclegg@veritasgenetics.com>
Date: Wed, 20 Jun 2018 13:25:25 +0000 (-0400)
Subject: Merge branch '13164-fix-zero-priority-after-race'
X-Git-Tag: 1.2.0~105
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/9fa635c9dc288317f19471291accecf8690f5718?hp=f9e94997cb5c2166d8b71874f263544cfc2fe5ba

Merge branch '13164-fix-zero-priority-after-race'

refs #13164

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>
---

diff --git a/services/api/lib/update_priority.rb b/services/api/lib/update_priority.rb
index 724d2b20a8..21cd74bae6 100644
--- a/services/api/lib/update_priority.rb
+++ b/services/api/lib/update_priority.rb
@@ -3,8 +3,15 @@
 # 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.
+  extend CurrentApiClient
+
+  # Clean up after races.
+  #
+  # If container priority>0 but there are no committed container
+  # requests for it, reset priority to 0.
+  #
+  # If container priority=0 but there are committed container requests
+  # for it with priority>0, update priority.
   def self.update_priority
     if !File.owned?(Rails.root.join('tmp'))
       Rails.logger.warn("UpdatePriority: not owner of #{Rails.root}/tmp, skipping")
@@ -13,7 +20,19 @@ module UpdatePriority
     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);")
+
+      # priority>0 but should be 0:
+      ActiveRecord::Base.connection.
+        exec_query("UPDATE containers AS c SET priority=0 WHERE state IN ('Queued', 'Locked', 'Running') AND priority>0 AND uuid NOT IN (SELECT container_uuid FROM container_requests WHERE priority>0 AND state='Committed');", 'UpdatePriority')
+
+      # priority==0 but should be >0:
+      act_as_system_user do
+        Container.
+          joins("JOIN container_requests ON container_requests.container_uuid=containers.uuid AND container_requests.state=#{Container.sanitize(ContainerRequest::Committed)} AND container_requests.priority>0").
+          where('containers.state IN (?) AND containers.priority=0 AND container_requests.uuid IS NOT NULL',
+                [Container::Queued, Container::Locked, Container::Running]).
+          map(&:update_priority!)
+      end
     end
   end
 
diff --git a/services/api/test/unit/update_priority_test.rb b/services/api/test/unit/update_priority_test.rb
new file mode 100644
index 0000000000..2d28d3fb69
--- /dev/null
+++ b/services/api/test/unit/update_priority_test.rb
@@ -0,0 +1,30 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'test_helper'
+require 'update_priority'
+
+class UpdatePriorityTest < ActiveSupport::TestCase
+  test 'priority 0 but should be >0' do
+    uuid = containers(:running).uuid
+    ActiveRecord::Base.connection.exec_query('UPDATE containers SET priority=0 WHERE uuid=$1', 'test-setup', [[nil, uuid]])
+    assert_equal 0, Container.find_by_uuid(uuid).priority
+    UpdatePriority.update_priority
+    assert_operator 0, :<, Container.find_by_uuid(uuid).priority
+
+    uuid = containers(:queued).uuid
+    ActiveRecord::Base.connection.exec_query('UPDATE containers SET priority=0 WHERE uuid=$1', 'test-setup', [[nil, uuid]])
+    assert_equal 0, Container.find_by_uuid(uuid).priority
+    UpdatePriority.update_priority
+    assert_operator 0, :<, Container.find_by_uuid(uuid).priority
+  end
+
+  test 'priority>0 but should be 0' do
+    uuid = containers(:running).uuid
+    ActiveRecord::Base.connection.exec_query('DELETE FROM container_requests WHERE container_uuid=$1', 'test-setup', [[nil, uuid]])
+    assert_operator 0, :<, Container.find_by_uuid(uuid).priority
+    UpdatePriority.update_priority
+    assert_equal 0, Container.find_by_uuid(uuid).priority
+  end
+end