From d30cad367216981766344fea34c3d439bafbc8f5 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Thu, 4 May 2023 16:11:49 -0400 Subject: [PATCH] 20472: Always do "select for update" before priority update Code cleanup. Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- .../v1/container_requests_controller.rb | 7 +-- .../arvados/v1/containers_controller.rb | 4 +- ...0230503224107_priority_update_functions.rb | 7 ++- services/api/db/structure.sql | 49 +++++++++---------- services/api/lib/update_priorities.rb | 17 ++++--- 5 files changed, 43 insertions(+), 41 deletions(-) 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 69729162fa..b57f09a6a1 100644 --- 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 9247327acd..ceb40348bb 100644 --- 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 df6f9d0bb5..3504a10691 100644 --- 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 ffb581baf3..59af57f75c 100644 --- 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 b7799cb4bd..b59859a82a 100644 --- 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 -- 2.30.2