From: Peter Amstutz <peter.amstutz@curii.com>
Date: Fri, 19 May 2023 19:01:34 +0000 (-0400)
Subject: 20529: Lock direct parent containers on updates
X-Git-Tag: 2.7.0~103^2
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/f9fa3a5e585a49b3673749bc235ad881707762b9

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 b57f09a6a1..6b6e96a1f7 100644
--- 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 17970ce4cd..13aa478d26 100644
--- 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 b59859a82a..4183ac10b1 100644
--- 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 8c2919b971..07fa5c3211 100644
--- 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