20529: Lock direct parent containers on updates

This is because cost is propagated up to the parent container on
container completion.

Handle case of resource attrs at top level when deciding whether to
lock or not (resource attrs appear as strings, not symbols)

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