16007: Migration works with large database

Changing individual permissions on groups that affect a lot of objects
is expensive.  The migration now suppresses permission updates and
does a batch update at the end.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

diff --git a/services/api/db/migrate/20200602141328_fix_roles_projects.rb b/services/api/db/migrate/20200602141328_fix_roles_projects.rb
index f9b2bd6faf2e0e8ec4041632911e899d96116cf6..e17ef6de53e8f804ed711af4dd88d576ad30a3b9 100644 (file)
--- a/services/api/db/migrate/20200602141328_fix_roles_projects.rb
+++ b/services/api/db/migrate/20200602141328_fix_roles_projects.rb
@@ -5,9 +5,13 @@
 require 'fix_roles_projects'
 
 class FixRolesProjects < ActiveRecord::Migration[5.0]
-  def change
+  def up
+    # defined in a function for easy testing.
+    fix_roles_projects
+  end
+
+  def down
     # This migration is not reversible.  However, the results are
     # backwards compatible.
-    fix_roles_projects
   end
 end

diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 469475ad9604232fbeafc76889c4665b6cd77b80..83987d051859843a8df981cf539f8514daecc15b 100644 (file)
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -3196,6 +3196,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20190808145904'),
 ('20190809135453'),
 ('20190905151603'),
-('20200501150153');
+('20200501150153'),
+('20200602141328');
 
 

diff --git a/services/api/lib/fix_roles_projects.rb b/services/api/lib/fix_roles_projects.rb
index dafef61aa93ea8136509e58049f04339e15551eb..448c50cee212046b6949c5ee30c701706eba77b2 100644 (file)
--- a/services/api/lib/fix_roles_projects.rb
+++ b/services/api/lib/fix_roles_projects.rb
@@ -2,23 +2,25 @@
 #
 # SPDX-License-Identifier: AGPL-3.0
 
+include CurrentApiClient
+
 def fix_roles_projects
-  # This migration is not reversible.  However, the behavior it
-  # enforces is backwards-compatible, and most of the time there
-  # shouldn't be anything to do at all.
-  act_as_system_user do
-    ActiveRecord::Base.transaction do
-      q = ActiveRecord::Base.connection.exec_query %{
+  batch_update_permissions do
+    # This migration is not reversible.  However, the behavior it
+    # enforces is backwards-compatible, and most of the time there
+    # shouldn't be anything to do at all.
+    act_as_system_user do
+      ActiveRecord::Base.transaction do
+        q = ActiveRecord::Base.connection.exec_query %{
 select uuid from groups limit 1
 }
 
-      # 1) any group not group_class != project becomes a 'role' (both empty and invalid groups)
-      ActiveRecord::Base.connection.exec_query %{
+        # 1) any group not group_class != project becomes a 'role' (both empty and invalid groups)
+        ActiveRecord::Base.connection.exec_query %{
 UPDATE groups set group_class='role' where group_class != 'project' or group_class is null
     }
 
-      Group.where(group_class: "role").each do |g|
-        if g.owner_uuid != system_user_uuid
+        Group.where("group_class='role' and owner_uuid != '#{system_user_uuid}'").each do |g|
           # 2) Ownership of a role becomes a can_manage link
           Link.create!(link_class: 'permission',
                        name: 'can_manage',
@@ -28,35 +30,46 @@ UPDATE groups set group_class='role' where group_class != 'project' or group_cla
           g.save_with_unique_name!
         end
 
-        # 3) If a role owns anything, give it to system user and it
-        # becomes a can_manage link
         ActiveRecord::Base.descendants.reject(&:abstract_class?).each do |klass|
           next if [ApiClientAuthorization,
                    AuthorizedKey,
-                   Log].include?(klass)
+                   Log,
+                   Group].include?(klass)
           next if !klass.columns.collect(&:name).include?('owner_uuid')
 
-          klass.where(owner_uuid: g.uuid).each do |owned|
+          # 3) If a role owns anything, give it to system user and it
+          # becomes a can_manage link
+          klass.joins("join groups on groups.uuid=#{klass.table_name}.owner_uuid and groups.group_class='role'").each do |owned|
             Link.create!(link_class: 'permission',
                          name: 'can_manage',
-                         tail_uuid: g.uuid,
+                         tail_uuid: owned.owner_uuid,
                          head_uuid: owned.uuid)
             owned.owner_uuid = system_user_uuid
             owned.save_with_unique_name!
           end
         end
-      end
 
-      # 4) Projects can't have outgoing permission links.  Just delete them.
-      q = ActiveRecord::Base.connection.exec_query %{
+        Group.joins("join groups as g2 on g2.uuid=groups.owner_uuid and g2.group_class='role'").each do |owned|
+          Link.create!(link_class: 'permission',
+                       name: 'can_manage',
+                       tail_uuid: owned.owner_uuid,
+                       head_uuid: owned.uuid)
+          owned.owner_uuid = system_user_uuid
+          owned.save_with_unique_name!
+        end
+
+        # 4) Projects can't have outgoing permission links.  Just
+        # print a warning and delete them.
+        q = ActiveRecord::Base.connection.exec_query %{
 select links.uuid from links, groups where groups.uuid = links.tail_uuid and
        links.link_class = 'permission' and groups.group_class = 'project'
 }
-      q.each do |lu|
-        ln = Link.find_by_uuid(lu['uuid'])
-        puts "Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed"
-        Rails.logger.warn "Destroying invalid permission link from project #{ln.tail_uuid} to #{ln.head_uuid}"
-        ln.destroy!
+        q.each do |lu|
+          ln = Link.find_by_uuid(lu['uuid'])
+          puts "WARNING: Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed"
+          Rails.logger.warn "Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed"
+          ln.destroy!
+        end
       end
     end
   end

diff --git a/services/api/lib/update_permissions.rb b/services/api/lib/update_permissions.rb
index 4c2e72d9553c3b6e110c9c3c6a0360afebe64390..7b1b900cacbcae00a4d44ce5d8f72d02b213feb3 100644 (file)
--- a/services/api/lib/update_permissions.rb
+++ b/services/api/lib/update_permissions.rb
@@ -8,6 +8,8 @@ REVOKE_PERM = 0
 CAN_MANAGE_PERM = 3
 
 def update_permissions perm_origin_uuid, starting_uuid, perm_level, edge_id=nil
+  return if Thread.current[:suppress_update_permissions]
+
   #
   # Update a subset of the permission table affected by adding or
   # removing a particular permission relationship (ownership or a
@@ -140,7 +142,7 @@ end
 
 def check_permissions_against_full_refresh
   # No-op except when running tests
-  return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh]
+  return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh] and !Thread.current[:suppress_update_permissions]
 
   # For checking correctness of the incremental permission updates.
   # Check contents of the current 'materialized_permission' table
@@ -191,6 +193,17 @@ def skip_check_permissions_against_full_refresh
   end
 end
 
+def batch_update_permissions
+  check_perm_was = Thread.current[:suppress_update_permissions]
+  Thread.current[:suppress_update_permissions] = true
+  begin
+    yield
+  ensure
+    Thread.current[:suppress_update_permissions] = check_perm_was
+    refresh_permissions
+  end
+end
+
 # Used to account for permissions that a user gains by having
 # can_manage on another user.
 #