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 + 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 + # 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 + 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