[arvados] updated: 2.6.0-101-ge881f3054
git repository hosting
git at public.arvados.org
Thu May 4 03:12:31 UTC 2023
Summary of changes:
.../arvados/v1/container_requests_controller.rb | 16 +++++
.../arvados/v1/containers_controller.rb | 23 +++++--
services/api/app/models/container.rb | 1 +
.../20230503224107_priority_update_functions.rb | 39 ++++++++++--
services/api/db/structure.sql | 71 +++++++++++++++++++++-
services/api/lib/update_priorities.rb | 10 ++-
services/api/test/unit/container_request_test.rb | 10 +++
7 files changed, 157 insertions(+), 13 deletions(-)
via e881f30542392c8328909139a94609230aee9975 (commit)
via 71b1fd09907996f5e68c467b17320ef98458852d (commit)
via d4f7f5e03f637ee5426dded188beb08fcf4fa7bc (commit)
from 1c7e8986e9a71b693e65f91ce0f3482260c5c96b (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 e881f30542392c8328909139a94609230aee9975
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Wed May 3 23:11:14 2023 -0400
20472: Add a few comments and add container_tree function
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/db/migrate/20230503224107_priority_update_functions.rb b/services/api/db/migrate/20230503224107_priority_update_functions.rb
index dc9717f67..df6f9d0bb 100644
--- a/services/api/db/migrate/20230503224107_priority_update_functions.rb
+++ b/services/api/db/migrate/20230503224107_priority_update_functions.rb
@@ -8,6 +8,10 @@ class PriorityUpdateFunctions < ActiveRecord::Migration[5.2]
CREATE OR REPLACE FUNCTION container_priority(for_container_uuid character varying, inherited bigint, inherited_from character varying) returns bigint
LANGUAGE sql
AS $$
+/* Determine the priority of an individual container.
+ 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
when containers.priority is not NULL then containers.priority
@@ -22,6 +26,10 @@ $$;
CREATE OR REPLACE FUNCTION 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
@@ -32,11 +40,31 @@ union
)
select upd_container_uuid, upd_priority from tab;
$$;
+}
+
+ ActiveRecord::Base.connection.execute %{
+CREATE OR REPLACE FUNCTION container_tree(for_container_uuid character varying) returns table (pri_container_uuid character varying)
+ LANGUAGE sql
+ AS $$
+/* A lighter weight version of the update_priorities query that only returns the containers in a tree,
+ used by SELECT FOR UPDATE.
+*/
+with recursive tab(upd_container_uuid) as (
+ select for_container_uuid
+union
+ select containers.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 from tab;
+$$;
}
end
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"
end
end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 1b0eab5f6..ffb581baf 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -197,6 +197,10 @@ $$;
CREATE FUNCTION public.container_priority(for_container_uuid character varying, inherited bigint, inherited_from character varying) RETURNS bigint
LANGUAGE sql
AS $$
+/* Determine the priority of an individual container.
+ 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
when containers.priority is not NULL then containers.priority
@@ -207,6 +211,28 @@ select coalesce(max(case when container_requests.priority = 0 then 0
$$;
+--
+-- Name: container_tree(character varying); Type: FUNCTION; Schema: public; Owner: -
+--
+
+CREATE FUNCTION public.container_tree(for_container_uuid character varying) RETURNS TABLE(pri_container_uuid character varying)
+ LANGUAGE sql
+ AS $$
+/* A lighter weight version of the update_priorities query that only returns the containers in a tree,
+ used by SELECT FOR UPDATE.
+*/
+with recursive tab(upd_container_uuid) as (
+ select for_container_uuid
+union
+ select containers.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 from tab;
+$$;
+
+
--
-- Name: project_subtree_with_is_frozen(character varying, boolean); Type: FUNCTION; Schema: public; Owner: -
--
@@ -276,6 +302,10 @@ $$;
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
diff --git a/services/api/lib/update_priorities.rb b/services/api/lib/update_priorities.rb
index bd5b71b90..b7799cb4b 100644
--- a/services/api/lib/update_priorities.rb
+++ b/services/api/lib/update_priorities.rb
@@ -5,12 +5,12 @@
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
+ 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 update_priorities($1)) order by containers.uuid for update
+ 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
commit 71b1fd09907996f5e68c467b17320ef98458852d
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Wed May 3 22:58:15 2023 -0400
20472: Inherit priority being propagated down
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 afddd5686..69729162f 100644
--- a/services/api/app/controllers/arvados/v1/container_requests_controller.rb
+++ b/services/api/app/controllers/arvados/v1/container_requests_controller.rb
@@ -2,6 +2,8 @@
#
# SPDX-License-Identifier: AGPL-3.0
+require 'update_priorities'
+
class Arvados::V1::ContainerRequestsController < ApplicationController
accept_attribute_as_json :environment, Hash
accept_attribute_as_json :mounts, Hash
@@ -29,4 +31,18 @@ class Arvados::V1::ContainerRequestsController < ApplicationController
})
end
+ 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.
+ super
+ else
+ # Lock containers table to avoid deadlock in cascading priority update (see #20240)
+ Container.transaction do
+ row_lock_for_priority_update @object.container_uuid
+ super
+ end
+ end
+ end
end
diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index a66271f8c..8e36cdc0d 100644
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -2,6 +2,8 @@
#
# SPDX-License-Identifier: AGPL-3.0
+require 'update_priorities'
+
class Arvados::V1::ContainersController < ApplicationController
accept_attribute_as_json :environment, Hash
accept_attribute_as_json :mounts, Hash
@@ -28,6 +30,22 @@ class Arvados::V1::ContainersController < ApplicationController
show
end
+ 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.
+ super
+ else
+ Container.transaction do
+ # Get locks ahead of time to avoid deadlock in cascading priority
+ # update
+ row_lock_for_priority_update @object.uuid
+ super
+ end
+ end
+ end
+
def find_objects_for_index
super
if action_name == 'lock' || action_name == 'unlock'
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 6301fe808..d897ff7af 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -130,6 +130,7 @@ class Container < ArvadosModel
# user-assigned priority and request creation time.
def update_priority!
update_priorities uuid
+ reload
end
# Create a new container (or find an existing one) to satisfy the
diff --git a/services/api/db/migrate/20230503224107_priority_update_functions.rb b/services/api/db/migrate/20230503224107_priority_update_functions.rb
index fbb3e27bf..dc9717f67 100644
--- a/services/api/db/migrate/20230503224107_priority_update_functions.rb
+++ b/services/api/db/migrate/20230503224107_priority_update_functions.rb
@@ -5,11 +5,12 @@
class PriorityUpdateFunctions < ActiveRecord::Migration[5.2]
def up
ActiveRecord::Base.connection.execute %{
-CREATE OR REPLACE FUNCTION container_priority(for_container_uuid character varying, inherited bigint) returns bigint
+CREATE OR REPLACE FUNCTION container_priority(for_container_uuid character varying, inherited bigint, inherited_from character varying) returns bigint
LANGUAGE sql
AS $$
select coalesce(max(case when container_requests.priority = 0 then 0
- when containers.priority is not NULL then greatest(containers.priority, inherited)
+ 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
container_requests left outer join containers on container_requests.requesting_container_uuid = containers.uuid
@@ -18,13 +19,13 @@ $$;
}
ActiveRecord::Base.connection.execute %{
-CREATE OR REPLACE FUNCTION update_priorities(for_container_uuid character varying) returns table (pri_container_uuid character varying, priority bigint)
+CREATE OR REPLACE FUNCTION update_priorities(for_container_uuid character varying) returns table (pri_container_uuid character varying, upd_priority bigint)
LANGUAGE sql
AS $$
with recursive tab(upd_container_uuid, upd_priority) as (
- select for_container_uuid, container_priority(for_container_uuid, 0)
+ select for_container_uuid, container_priority(for_container_uuid, 0, '')
union
- select containers.uuid, container_priority(containers.uuid, child_requests.upd_priority)
+ 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')
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 3dd705a80..1b0eab5f6 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -190,6 +190,23 @@ case (edges.edge_id = perm_edge_id)
$$;
+--
+-- Name: container_priority(character varying, bigint, character varying); Type: FUNCTION; Schema: public; Owner: -
+--
+
+CREATE FUNCTION public.container_priority(for_container_uuid character varying, inherited bigint, inherited_from character varying) RETURNS bigint
+ LANGUAGE sql
+ AS $$
+select coalesce(max(case when container_requests.priority = 0 then 0
+ 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
+ container_requests left outer join containers on container_requests.requesting_container_uuid = containers.uuid
+ where container_requests.container_uuid = for_container_uuid and container_requests.state = 'Committed' and container_requests.priority > 0;
+$$;
+
+
--
-- Name: project_subtree_with_is_frozen(character varying, boolean); Type: FUNCTION; Schema: public; Owner: -
--
@@ -252,8 +269,29 @@ 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 $$
+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;
+
--
-- Name: api_client_authorizations; Type: TABLE; Schema: public; Owner: -
--
@@ -3202,6 +3240,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20220804133317'),
('20221219165512'),
('20221230155924'),
-('20230421142716');
+('20230421142716'),
+('20230503224107');
diff --git a/services/api/lib/update_priorities.rb b/services/api/lib/update_priorities.rb
index 66d307436..bd5b71b90 100644
--- a/services/api/lib/update_priorities.rb
+++ b/services/api/lib/update_priorities.rb
@@ -4,7 +4,13 @@
def update_priorities starting_container_uuid
ActiveRecord::Base.connection.exec_query %{
-update containers set priority=computed.priority from (select pri_container_uuid, priority from update_priorities($1) order by pri_container_uuid) as computed
+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
}, '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 update_priorities($1)) order by containers.uuid for update
+ }, 'select_for_update_priorities', [[nil, container_uuid]]
+end
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 006bb7941..931176b8b 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -466,6 +466,16 @@ class ContainerRequestTest < ActiveSupport::TestCase
assert_operator shared_grandchild.priority, :<=, grandchildren[2].priority
assert_operator shared_grandchild.priority, :<=, children[2].priority
assert_operator shared_grandchild.priority, :<=, parents[2].priority
+
+ # cancelling the most recent toplevel container should
+ # reprioritize all of its descendants (except the shared
+ # grandchild) to zero
+ toplevel_crs[2].update_attributes!(priority: 0)
+ (parents + children + grandchildren + [shared_grandchild]).map(&:reload)
+ assert_operator 0, :==, parents[2].priority
+ assert_operator 0, :==, children[2].priority
+ assert_operator 0, :==, grandchildren[2].priority
+ assert_operator shared_grandchild.priority, :==, grandchildren[0].priority
end
[
commit d4f7f5e03f637ee5426dded188beb08fcf4fa7bc
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Wed May 3 21:31:06 2023 -0400
20472: Remove special handling of update_priority
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index b7dc03022..a66271f8c 100644
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -34,11 +34,6 @@ class Arvados::V1::ContainersController < ApplicationController
# Avoid loading more fields than we need
@objects = @objects.select(:id, :uuid, :state, :priority, :auth_uuid, :locked_by_uuid, :lock_count)
@select = %w(uuid state priority auth_uuid locked_by_uuid)
- elsif action_name == 'update_priority'
- # We're going to reload(lock: true) in the handler, which will
- # select all attributes, but will fail if we don't select :id
- # now.
- @objects = @objects.select(:id, :uuid)
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list