Merge branch 'refresh-authorizations-with-lease' into 'master'

Refresh project authorizations using a Redis lease

This MR changes `User#refresh_authorized_projects` so it uses a Redis lease instead of relying on serializable transactions. See the commit message(s) for more details.

See merge request !7733

10 files changed, 84 insertions, 48 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 62502efc7b4..b54ce14f0bf 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