Merge branch '20529-container-deadlocks' refs #20529

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

diff --git a/services/api/app/controllers/arvados/v1/container_requests_controller.rb b/services/api/app/controllers/arvados/v1/container_requests_controller.rb
index b57f09a6a1ee2748d6ca40a43ea709cd0e64b4d8..6b6e96a1f71cb4f86444c74d720a8eb19711004a 100644 (file)
--- a/services/api/app/controllers/arvados/v1/container_requests_controller.rb
+++ b/services/api/app/controllers/arvados/v1/container_requests_controller.rb
@@ -32,7 +32,7 @@ class Arvados::V1::ContainerRequestsController < ApplicationController
   end
 
   def update
-    if (resource_attrs.keys - [:owner_uuid, :name, :description, :properties]).empty? or @object.container_uuid.nil?
+    if (resource_attrs.keys.map(&:to_sym) - [:owner_uuid, :name, :description, :properties]).empty? or @object.container_uuid.nil?
       # If no attributes are being updated besides these, there are no
       # cascading changes to other rows/tables, the only lock will be
       # the single row lock on SQL UPDATE.

diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index 17970ce4cd271c13d6f66c2ee59b1a04da76ce3a..13aa478d26b15bc882adb6b38e4f012db5fa1145 100644 (file)
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -31,7 +31,7 @@ class Arvados::V1::ContainersController < ApplicationController
   end
 
   def update
-    if (resource_attrs.keys - [:cost, :gateway_address, :output_properties, :progress, :runtime_status]).empty?
+    if (resource_attrs.keys.map(&:to_sym) - [:cost, :gateway_address, :output_properties, :progress, :runtime_status]).empty?
       # If no attributes are being updated besides these, there are no
       # cascading changes to other rows/tables, the only lock will the
       # single row lock on SQL UPDATE.

diff --git a/services/api/lib/update_priorities.rb b/services/api/lib/update_priorities.rb
index b59859a82a529fffe72578e69f37c87dd527e542..4183ac10b15bed9436022618b7f563c53e9938c0 100644 (file)
--- a/services/api/lib/update_priorities.rb
+++ b/services/api/lib/update_priorities.rb
@@ -3,8 +3,20 @@
 # SPDX-License-Identifier: AGPL-3.0
 
 def row_lock_for_priority_update container_uuid
+  # Locks all the containers under this container, and also any
+  # immediate parent containers.  This ensures we have locked
+  # everything that gets touched by either a priority update or state
+  # update.
   ActiveRecord::Base.connection.exec_query %{
-        select 1 from containers where containers.uuid in (select pri_container_uuid from container_tree($1)) order by containers.uuid for update
+        select 1 from containers where containers.uuid in (
+  select pri_container_uuid from container_tree($1)
+UNION
+  select container_requests.requesting_container_uuid from container_requests
+    where container_requests.container_uuid = $1
+          and container_requests.state = 'Committed'
+          and container_requests.requesting_container_uuid is not NULL
+)
+        order by containers.uuid for update
   }, 'select_for_update_priorities', [[nil, container_uuid]]
 end
 

diff --git a/services/api/test/functional/arvados/v1/containers_controller_test.rb b/services/api/test/functional/arvados/v1/containers_controller_test.rb
index 8c2919b97102db2e3007938e1fe32405c4251928..07fa5c3211c3b74f3ba5ccfa0756ecf57566322f 100644 (file)
--- a/services/api/test/functional/arvados/v1/containers_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/containers_controller_test.rb
@@ -168,4 +168,25 @@ class Arvados::V1::ContainersControllerTest < ActionController::TestCase
     assert_response :success
     assert_not_equal 0, Container.find_by_uuid(containers(:running).uuid).priority
   end
+
+  test 'update runtime_status, runtime_status is toplevel key' do
+    authorize_with :dispatch1
+    c = containers(:running)
+    patch :update, params: {id: containers(:running).uuid, runtime_status: {activity: "foo", activityDetail: "bar"}}
+    assert_response :success
+  end
+
+  test 'update runtime_status, container is toplevel key' do
+    authorize_with :dispatch1
+    c = containers(:running)
+    patch :update, params: {id: containers(:running).uuid, container: {runtime_status: {activity: "foo", activityDetail: "bar"}}}
+    assert_response :success
+  end
+
+  test 'update state, state is toplevel key' do
+    authorize_with :dispatch1
+    c = containers(:running)
+    patch :update, params: {id: containers(:running).uuid, state: "Complete", runtime_status: {activity: "finishing"}}
+    assert_response :success
+  end
 end