Refresh project authorizations using a Redis leaserefresh-authorizations-with-lease

When I proposed using serializable transactions I was hoping we would be
able to refresh data of individual users concurrently. Unfortunately
upon closer inspection it was revealed this was not the case. This could
result in a lot of queries failing due to serialization errors,
overloading the database in the process (given enough workers trying to
update the target table).

To work around this we're now using a Redis lease that is cancelled upon
completion. This ensures we can update the data of different users
concurrently without overloading the database.

The code will try to obtain the lease until it succeeds, waiting at
least 1 second between retries. This is necessary as we may otherwise
end up _not_ updating the data which is not an option.

10 files changed, 84 insertions, 48 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 513a19d81d2..ad4b4d9381b 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -445,27 +445,21 @@ class User < ActiveRecord::Base
   end
 
   def refresh_authorized_projects
-    loop do
-      begin
-        Gitlab::Database.serialized_transaction do
-          project_authorizations.delete_all
-
-          # project_authorizations_union can return multiple records for the same project/user with
-          # different access_level so we take row with the maximum access_level
-          project_authorizations.connection.execute <<-SQL
-            INSERT INTO project_authorizations (user_id, project_id, access_level)
-            SELECT user_id, project_id, MAX(access_level) AS access_level
-            FROM (#{project_authorizations_union.to_sql}) sub
-            GROUP BY user_id, project_id
-          SQL
-
-          update_column(:authorized_projects_populated, true) unless authorized_projects_populated
-        end
-
-        break
-      # In the event of a concurrent modification Rails raises StatementInvalid.
-      # In this case we want to keep retrying until the transaction succeeds
-      rescue ActiveRecord::StatementInvalid
+    transaction do
+      project_authorizations.delete_all
+
+      # project_authorizations_union can return multiple records for the same
+      # project/user with different access_level so we take row with the maximum
+      # access_level
+      project_authorizations.connection.execute <<-SQL
+      INSERT INTO project_authorizations (user_id, project_id, access_level)
+      SELECT user_id, project_id, MAX(access_level) AS access_level
+      FROM (#{project_authorizations_union.to_sql}) sub
+      GROUP BY user_id, project_id
+      SQL
+
+      unless authorized_projects_populated
+        update_column(:authorized_projects_populated, true)
       end
     end
   end
diff --git a/app/workers/authorized_projects_worker.rb b/app/workers/authorized_projects_worker.rb
index 331727ba9d8..fccddb70d18 100644
--- a/app/workers/authorized_projects_worker.rb
+++ b/app/workers/authorized_projects_worker.rb
@@ -2,14 +2,33 @@ class AuthorizedProjectsWorker
   include Sidekiq::Worker
   include DedicatedSidekiqQueue
 
+  LEASE_TIMEOUT = 1.minute.to_i
+
   def self.bulk_perform_async(args_list)
     Sidekiq::Client.push_bulk('class' => self, 'args' => args_list)
   end
 
   def perform(user_id)
     user = User.find_by(id: user_id)
-    return unless user
 
-    user.refresh_authorized_projects
+    refresh(user) if user
+  end
+
+  def refresh(user)
+    lease_key = "refresh_authorized_projects:#{user.id}"
+    lease = Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT)
+
+    until uuid = lease.try_obtain
+      # Keep trying until we obtain the lease. If we don't do so we may end up
+      # not updating the list of authorized projects properly. To prevent
+      # hammering Redis too much we'll wait for a bit between retries.
+      sleep(1)
+    end
+
+    begin
+      user.refresh_authorized_projects
+    ensure
+      Gitlab::ExclusiveLease.cancel(lease_key, uuid)
+    end
   end
 end
diff --git a/changelogs/unreleased/refresh-authorizations-with-lease.yml b/changelogs/unreleased/refresh-authorizations-with-lease.yml
new file mode 100644
index 00000000000..bb9b77018e3
--- /dev/null
+++ b/changelogs/unreleased/refresh-authorizations-with-lease.yml
@@ -0,0 +1,4 @@
+---
+title: Use a Redis lease for updating authorized projects
+merge_request: 7733
+author: 
diff --git a/db/fixtures/development/04_project.rb b/db/fixtures/development/04_project.rb
index 18a2df7c059..a984eda5ab5 100644
--- a/db/fixtures/development/04_project.rb
+++ b/db/fixtures/development/04_project.rb
@@ -1,5 +1,4 @@
 require 'sidekiq/testing'
-require './db/fixtures/support/serialized_transaction'
 
 Sidekiq::Testing.inline! do
   Gitlab::Seeder.quiet do
diff --git a/db/fixtures/development/06_teams.rb b/db/fixtures/development/06_teams.rb
index 04c3690e152..5c2a03fec3f 100644
--- a/db/fixtures/development/06_teams.rb
+++ b/db/fixtures/development/06_teams.rb
@@ -1,5 +1,4 @@
 require 'sidekiq/testing'
-require './db/fixtures/support/serialized_transaction'
 
 Sidekiq::Testing.inline! do
   Gitlab::Seeder.quiet do
diff --git a/db/fixtures/development/17_cycle_analytics.rb b/db/fixtures/development/17_cycle_analytics.rb
index 7b3908fae98..916ee8dbac8 100644
--- a/db/fixtures/development/17_cycle_analytics.rb
+++ b/db/fixtures/development/17_cycle_analytics.rb
@@ -1,6 +1,5 @@
 require 'sidekiq/testing'
 require './spec/support/test_env'
-require './db/fixtures/support/serialized_transaction'
 
 class Gitlab::Seeder::CycleAnalytics
   def initialize(project, perf: false)
diff --git a/db/fixtures/support/serialized_transaction.rb b/db/fixtures/support/serialized_transaction.rb
deleted file mode 100644
index d3305b661e5..00000000000
--- a/db/fixtures/support/serialized_transaction.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-require 'gitlab/database'
-
-module Gitlab
-  module Database
-    def self.serialized_transaction
-      connection.transaction { yield }
-    end
-  end
-end
diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb
index 2d5c9232425..55b8f888d53 100644
--- a/lib/gitlab/database.rb
+++ b/lib/gitlab/database.rb
@@ -35,13 +35,6 @@ module Gitlab
       order
     end
 
-    def self.serialized_transaction
-      opts = {}
-      opts[:isolation] = :serializable unless Rails.env.test? && connection.transaction_open?
-
-      connection.transaction(opts) { yield }
-    end
-
     def self.random
       Gitlab::Database.postgresql? ? "RANDOM()" : "RAND()"
     end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 91826e5884d..14c891994d0 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1349,4 +1349,31 @@ describe User, models: true do
       expect(projects).to be_empty
     end
   end
+
+  describe '#refresh_authorized_projects', redis: true do
+    let(:project1) { create(:empty_project) }
+    let(:project2) { create(:empty_project) }
+    let(:user) { create(:user) }
+
+    before do
+      project1.team << [user, :reporter]
+      project2.team << [user, :guest]
+
+      user.project_authorizations.delete_all
+      user.refresh_authorized_projects
+    end
+
+    it 'refreshes the list of authorized projects' do
+      expect(user.project_authorizations.count).to eq(2)
+    end
+
+    it 'sets the authorized_projects_populated column' do
+      expect(user.authorized_projects_populated).to eq(true)
+    end
+
+    it 'stores the correct access levels' do
+      expect(user.project_authorizations.where(access_level: Gitlab::Access::GUEST).exists?).to eq(true)
+      expect(user.project_authorizations.where(access_level: Gitlab::Access::REPORTER).exists?).to eq(true)
+    end
+  end
 end
diff --git a/spec/workers/authorized_projects_worker_spec.rb b/spec/workers/authorized_projects_worker_spec.rb
index 18a1aab766c..95e2458da35 100644
--- a/spec/workers/authorized_projects_worker_spec.rb
+++ b/spec/workers/authorized_projects_worker_spec.rb
@@ -1,22 +1,33 @@
 require 'spec_helper'
 
 describe AuthorizedProjectsWorker do
+  let(:worker) { described_class.new }
+
   describe '#perform' do
     it "refreshes user's authorized projects" do
       user = create(:user)
 
-      expect(User).to receive(:find_by).with(id: user.id).and_return(user)
-      expect(user).to receive(:refresh_authorized_projects)
+      expect(worker).to receive(:refresh).with(an_instance_of(User))
 
-      described_class.new.perform(user.id)
+      worker.perform(user.id)
     end
 
-    context "when user is not found" do
+    context "when the user is not found" do
       it "does nothing" do
-        expect_any_instance_of(User).not_to receive(:refresh_authorized_projects)
+        expect(worker).not_to receive(:refresh)
 
-        described_class.new.perform(999_999)
+        described_class.new.perform(-1)
       end
     end
   end
+
+  describe '#refresh', redis: true do
+    it 'refreshes the authorized projects of the user' do
+      user = create(:user)
+
+      expect(user).to receive(:refresh_authorized_projects)
+
+      worker.refresh(user)
+    end
+  end
 end