[arvados] created: 2.6.0-171-gf9fa3a5e5

[git repository hosting]

        at  f9fa3a5e585a49b3673749bc235ad881707762b9 (commit)


commit f9fa3a5e585a49b3673749bc235ad881707762b9
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Fri May 19 15:01:34 2023 -0400

    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 at 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 b57f09a6a..6b6e96a1f 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 17970ce4c..13aa478d2 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 b59859a82..4183ac10b 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 8c2919b97..07fa5c321 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

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


hooks/post-receive
--