20472: Always do "select for update" before priority update

Code cleanup.

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 69729162fab4fd99405f1e3d973668f1f15e39e4..b57f09a6a1ee2748d6ca40a43ea709cd0e64b4d8 100644 (file)
--- a/services/api/app/controllers/arvados/v1/container_requests_controller.rb
+++ b/services/api/app/controllers/arvados/v1/container_requests_controller.rb
@@ -34,11 +34,12 @@ class Arvados::V1::ContainerRequestsController < ApplicationController
   def update
     if (resource_attrs.keys - [: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, so we should just use
-      # row locking.
+      # cascading changes to other rows/tables, the only lock will be
+      # the single row lock on SQL UPDATE.
       super
     else
-      # Lock containers table to avoid deadlock in cascading priority update (see #20240)
+      # Get locks ahead of time to avoid deadlock in cascading priority
+      # update
       Container.transaction do
         row_lock_for_priority_update @object.container_uuid
         super

diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index 9247327acd2f3583676ae8bacf5390795a4e95b3..ceb40348bbdfc723c30a5e9f99a76d0132ff6fe1 100644 (file)
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -33,8 +33,8 @@ class Arvados::V1::ContainersController < ApplicationController
   def update
     if (resource_attrs.keys - [: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, so we should just use
-      # row locking.
+      # cascading changes to other rows/tables, the only lock will the
+      # single row lock on SQL UPDATE.
       super
     else
       Container.transaction do

diff --git a/services/api/db/migrate/20230503224107_priority_update_functions.rb b/services/api/db/migrate/20230503224107_priority_update_functions.rb
index df6f9d0bb53adfd3bcfd4b51ba6c8cd0c6dd4651..3504a1069197f611cbc51642ba7590d818558d78 100644 (file)
--- a/services/api/db/migrate/20230503224107_priority_update_functions.rb
+++ b/services/api/db/migrate/20230503224107_priority_update_functions.rb
@@ -12,8 +12,7 @@ CREATE OR REPLACE FUNCTION container_priority(for_container_uuid character varyi
    The "inherited" priority comes from the path we followed from the root, the parent container
    priority hasn't been updated in the table yet but we need to behave it like it has been.
 */
-select coalesce(max(case when container_requests.priority = 0 then 0
-                         when containers.uuid = inherited_from then inherited
+select coalesce(max(case when containers.uuid = inherited_from then inherited
                          when containers.priority is not NULL then containers.priority
                          else container_requests.priority * 1125899906842624::bigint - (extract(epoch from container_requests.created_at)*1000)::bigint
                     end), 0) from
@@ -23,7 +22,7 @@ $$;
 }
 
     ActiveRecord::Base.connection.execute %{
-CREATE OR REPLACE FUNCTION update_priorities(for_container_uuid character varying) returns table (pri_container_uuid character varying, upd_priority bigint)
+CREATE OR REPLACE FUNCTION container_tree_priorities(for_container_uuid character varying) returns table (pri_container_uuid character varying, upd_priority bigint)
     LANGUAGE sql
     AS $$
 /* Calculate the priorities of all containers starting from for_container_uuid.
@@ -64,7 +63,7 @@ $$;
 
   def down
     ActiveRecord::Base.connection.execute "DROP FUNCTION container_priority"
-    ActiveRecord::Base.connection.execute "DROP FUNCTION update_priorities"
+    ActiveRecord::Base.connection.execute "DROP FUNCTION container_tree_priorities"
     ActiveRecord::Base.connection.execute "DROP FUNCTION container_tree"
   end
 end

diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index ffb581baf3f208760ddeb3772552f96f9ad43e5d..59af57f75cc2a8b545623b791677b125b377d2ae 100644 (file)
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -201,8 +201,7 @@ CREATE FUNCTION public.container_priority(for_container_uuid character varying,
    The "inherited" priority comes from the path we followed from the root, the parent container
    priority hasn't been updated in the table yet but we need to behave it like it has been.
 */
-select coalesce(max(case when container_requests.priority = 0 then 0
-                         when containers.uuid = inherited_from then inherited
+select coalesce(max(case when containers.uuid = inherited_from then inherited
                          when containers.priority is not NULL then containers.priority
                          else container_requests.priority * 1125899906842624::bigint - (extract(epoch from container_requests.created_at)*1000)::bigint
                     end), 0) from
@@ -233,6 +232,29 @@ select upd_container_uuid from tab;
 $$;
 
 
+--
+-- Name: container_tree_priorities(character varying); Type: FUNCTION; Schema: public; Owner: -
+--
+
+CREATE FUNCTION public.container_tree_priorities(for_container_uuid character varying) RETURNS TABLE(pri_container_uuid character varying, upd_priority bigint)
+    LANGUAGE sql
+    AS $$
+/* Calculate the priorities of all containers starting from for_container_uuid.
+   This traverses the process tree downward and calls container_priority for each container
+   and returns a table of container uuids and their new priorities.
+*/
+with recursive tab(upd_container_uuid, upd_priority) as (
+  select for_container_uuid, container_priority(for_container_uuid, 0, '')
+union
+  select containers.uuid, container_priority(containers.uuid, child_requests.upd_priority, child_requests.upd_container_uuid)
+  from (tab join container_requests on tab.upd_container_uuid = container_requests.requesting_container_uuid) as child_requests
+  join containers on child_requests.container_uuid = containers.uuid
+  where containers.state in ('Queued', 'Locked', 'Running')
+)
+select upd_container_uuid, upd_priority from tab;
+$$;
+
+
 --
 -- Name: project_subtree_with_is_frozen(character varying, boolean); Type: FUNCTION; Schema: public; Owner: -
 --
@@ -295,29 +317,6 @@ select starting_uuid like '_____-j7d0g-_______________' or
 $$;
 
 
---
--- Name: update_priorities(character varying); Type: FUNCTION; Schema: public; Owner: -
---
-
-CREATE FUNCTION public.update_priorities(for_container_uuid character varying) RETURNS TABLE(pri_container_uuid character varying, upd_priority bigint)
-    LANGUAGE sql
-    AS $$
-/* Calculate the priorities of all containers starting from for_container_uuid.
-   This traverses the process tree downward and calls container_priority for each container
-   and returns a table of container uuids and their new priorities.
-*/
-with recursive tab(upd_container_uuid, upd_priority) as (
-  select for_container_uuid, container_priority(for_container_uuid, 0, '')
-union
-  select containers.uuid, container_priority(containers.uuid, child_requests.upd_priority, child_requests.upd_container_uuid)
-  from (tab join container_requests on tab.upd_container_uuid = container_requests.requesting_container_uuid) as child_requests
-  join containers on child_requests.container_uuid = containers.uuid
-  where containers.state in ('Queued', 'Locked', 'Running')
-)
-select upd_container_uuid, upd_priority from tab;
-$$;
-
-
 SET default_tablespace = '';
 
 SET default_with_oids = false;

diff --git a/services/api/lib/update_priorities.rb b/services/api/lib/update_priorities.rb
index b7799cb4bd056632a38aa9ded363e1e3b0cc4c98..b59859a82a529fffe72578e69f37c87dd527e542 100644 (file)
--- a/services/api/lib/update_priorities.rb
+++ b/services/api/lib/update_priorities.rb
@@ -2,15 +2,18 @@
 #
 # SPDX-License-Identifier: AGPL-3.0
 
-def update_priorities starting_container_uuid
-  ActiveRecord::Base.connection.exec_query %{
-update containers set priority=computed.upd_priority from (select pri_container_uuid, upd_priority from update_priorities($1) order by pri_container_uuid) as computed
- where containers.uuid = computed.pri_container_uuid and priority != computed.upd_priority
-}, 'update_priorities', [[nil, starting_container_uuid]]
-end
-
 def row_lock_for_priority_update container_uuid
   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_for_update_priorities', [[nil, container_uuid]]
 end
+
+def update_priorities starting_container_uuid
+  # Ensure the row locks were taken in order
+  row_lock_for_priority_update starting_container_uuid
+
+  ActiveRecord::Base.connection.exec_query %{
+update containers set priority=computed.upd_priority from container_tree_priorities($1) as computed
+ where containers.uuid = computed.pri_container_uuid and priority != computed.upd_priority
+}, 'update_priorities', [[nil, starting_container_uuid]]
+end