[arvados] updated: 2.6.0-101-gd30cad367
git repository hosting
git at public.arvados.org
Thu May 4 20:13:57 UTC 2023
Summary of changes:
.../arvados/v1/container_requests_controller.rb | 7 ++--
.../arvados/v1/containers_controller.rb | 4 +-
.../20230503224107_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(-)
via d30cad367216981766344fea34c3d439bafbc8f5 (commit)
from d018aa211d3efc93c711248851db379b19fa60a1 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit d30cad367216981766344fea34c3d439bafbc8f5
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu May 4 16:11:49 2023 -0400
20472: Always do "select for update" before priority update
Code cleanup.
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 69729162f..b57f09a6a 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 9247327ac..ceb40348b 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 df6f9d0bb..3504a1069 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 ffb581baf..59af57f75 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 b7799cb4b..b59859a82 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
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list