Commit c9a7f475 authored by Douwe Maan's avatar Douwe Maan Committed by Alejandro Rodríguez

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
parent e505ab05
...@@ -442,13 +442,12 @@ class User < ActiveRecord::Base ...@@ -442,13 +442,12 @@ class User < ActiveRecord::Base
end end
def refresh_authorized_projects def refresh_authorized_projects
loop do transaction do
begin
Gitlab::Database.serialized_transaction do
project_authorizations.delete_all project_authorizations.delete_all
# project_authorizations_union can return multiple records for the same project/user with # project_authorizations_union can return multiple records for the same
# different access_level so we take row with the maximum access_level # project/user with different access_level so we take row with the maximum
# access_level
project_authorizations.connection.execute <<-SQL project_authorizations.connection.execute <<-SQL
INSERT INTO project_authorizations (user_id, project_id, access_level) INSERT INTO project_authorizations (user_id, project_id, access_level)
SELECT user_id, project_id, MAX(access_level) AS access_level SELECT user_id, project_id, MAX(access_level) AS access_level
...@@ -456,13 +455,8 @@ class User < ActiveRecord::Base ...@@ -456,13 +455,8 @@ class User < ActiveRecord::Base
GROUP BY user_id, project_id GROUP BY user_id, project_id
SQL SQL
update_column(:authorized_projects_populated, true) unless authorized_projects_populated unless authorized_projects_populated
end update_column(:authorized_projects_populated, true)
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
end end
end end
end end
......
...@@ -2,14 +2,33 @@ class AuthorizedProjectsWorker ...@@ -2,14 +2,33 @@ class AuthorizedProjectsWorker
include Sidekiq::Worker include Sidekiq::Worker
include DedicatedSidekiqQueue include DedicatedSidekiqQueue
LEASE_TIMEOUT = 1.minute.to_i
def self.bulk_perform_async(args_list) def self.bulk_perform_async(args_list)
Sidekiq::Client.push_bulk('class' => self, 'args' => args_list) Sidekiq::Client.push_bulk('class' => self, 'args' => args_list)
end end
def perform(user_id) def perform(user_id)
user = User.find_by(id: user_id) user = User.find_by(id: user_id)
return unless user
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 user.refresh_authorized_projects
ensure
Gitlab::ExclusiveLease.cancel(lease_key, uuid)
end
end end
end end
---
title: Use a Redis lease for updating authorized projects
merge_request: 7733
author:
require 'sidekiq/testing' require 'sidekiq/testing'
require './db/fixtures/support/serialized_transaction'
Sidekiq::Testing.inline! do Sidekiq::Testing.inline! do
Gitlab::Seeder.quiet do Gitlab::Seeder.quiet do
......
require 'sidekiq/testing' require 'sidekiq/testing'
require './spec/support/test_env' require './spec/support/test_env'
require './db/fixtures/support/serialized_transaction'
class Gitlab::Seeder::CycleAnalytics class Gitlab::Seeder::CycleAnalytics
def initialize(project, perf: false) def initialize(project, perf: false)
......
require 'gitlab/database'
module Gitlab
module Database
def self.serialized_transaction
connection.transaction { yield }
end
end
end
...@@ -35,13 +35,6 @@ module Gitlab ...@@ -35,13 +35,6 @@ module Gitlab
order order
end end
def self.serialized_transaction
opts = {}
opts[:isolation] = :serializable unless Rails.env.test? && connection.transaction_open?
connection.transaction(opts) { yield }
end
def self.random def self.random
Gitlab::Database.postgresql? ? "RANDOM()" : "RAND()" Gitlab::Database.postgresql? ? "RANDOM()" : "RAND()"
end end
......
...@@ -1338,4 +1338,31 @@ describe User, models: true do ...@@ -1338,4 +1338,31 @@ describe User, models: true do
expect(projects).to be_empty expect(projects).to be_empty
end end
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 end
require 'spec_helper' require 'spec_helper'
describe AuthorizedProjectsWorker do describe AuthorizedProjectsWorker do
let(:worker) { described_class.new }
describe '#perform' do describe '#perform' do
it "refreshes user's authorized projects" do it "refreshes user's authorized projects" do
user = create(:user) user = create(:user)
expect(User).to receive(:find_by).with(id: user.id).and_return(user) expect(worker).to receive(:refresh).with(an_instance_of(User))
expect(user).to receive(:refresh_authorized_projects)
described_class.new.perform(user.id) worker.perform(user.id)
end end
context "when user is not found" do context "when the user is not found" do
it "does nothing" 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
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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment