20472: Inherit priority being propagated down
authorPeter Amstutz <peter.amstutz@curii.com>
Thu, 4 May 2023 02:58:15 +0000 (22:58 -0400)
committerPeter Amstutz <peter.amstutz@curii.com>
Thu, 4 May 2023 14:42:00 +0000 (10:42 -0400)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

services/api/app/controllers/arvados/v1/container_requests_controller.rb
services/api/app/controllers/arvados/v1/containers_controller.rb
services/api/app/models/container.rb
services/api/db/migrate/20230503224107_priority_update_functions.rb
services/api/db/structure.sql
services/api/lib/update_priorities.rb
services/api/test/unit/container_request_test.rb

index afddd56864b42c2f92bdb105c5e1a3551068eae5..69729162fab4fd99405f1e3d973668f1f15e39e4 100644 (file)
@@ -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
index a66271f8c4cb7682c1399d740346ff1e8d4c7763..8e36cdc0d4e9fd788b12a119f57b00d0247aa769 100644 (file)
@@ -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'
index 6301fe8080c50015595574b69fe00c428c6c0bfb..d897ff7af9ea71b6a4e8694b9d2d23b0d9705c64 100644 (file)
@@ -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
index fbb3e27bf7d3eb26b757529f52bacac08d234d01..dc9717f67f49de54afd18d8cd5a339b38f4ae897 100644 (file)
@@ -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')
index 3dd705a801a8f29040da1b07f4fa0a3d45527c29..1b0eab5f67994bd628807c5ba9a33030278689c2 100644 (file)
@@ -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');
 
 
index 66d307436e29df7bdc408334287f41bc16c36dbe..bd5b71b90cd72447d932238679c3dab5392049ca 100644 (file)
@@ -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
index 006bb7941ff2208d6466955183b17ce7af4425fc..931176b8b40641e2c6fc81082679fe0c4be2cce2 100644 (file)
@@ -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
 
   [