Merge branch 'refresh-authorizations-tighter-lease' into 'master'
Synchronize all project authorization refreshing work using a lease Closes #25987 See merge request !8599
This commit is contained in:
commit
42399ac232
|
@ -26,8 +26,26 @@ module Users
|
||||||
user.reload
|
user.reload
|
||||||
end
|
end
|
||||||
|
|
||||||
# This method returns the updated User object.
|
|
||||||
def execute
|
def execute
|
||||||
|
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
|
||||||
|
execute_without_lease
|
||||||
|
ensure
|
||||||
|
Gitlab::ExclusiveLease.cancel(lease_key, uuid)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
# This method returns the updated User object.
|
||||||
|
def execute_without_lease
|
||||||
current = current_authorizations_per_project
|
current = current_authorizations_per_project
|
||||||
fresh = fresh_access_levels_per_project
|
fresh = fresh_access_levels_per_project
|
||||||
|
|
||||||
|
@ -47,26 +65,7 @@ module Users
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
update_with_lease(remove, add)
|
|
||||||
end
|
|
||||||
|
|
||||||
# Updates the list of authorizations using an exclusive lease.
|
|
||||||
def update_with_lease(remove = [], add = [])
|
|
||||||
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
|
|
||||||
update_authorizations(remove, add)
|
update_authorizations(remove, add)
|
||||||
ensure
|
|
||||||
Gitlab::ExclusiveLease.cancel(lease_key, uuid)
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|
||||||
# Updates the list of authorizations for the current user.
|
# Updates the list of authorizations for the current user.
|
||||||
|
|
|
@ -0,0 +1,4 @@
|
||||||
|
---
|
||||||
|
title: Synchronize all project authorization refreshing work to prevent race conditions
|
||||||
|
merge_request:
|
||||||
|
author:
|
|
@ -10,7 +10,21 @@ describe Users::RefreshAuthorizedProjectsService do
|
||||||
create!(project: project, user: user, access_level: access_level)
|
create!(project: project, user: user, access_level: access_level)
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '#execute' do
|
describe '#execute', :redis do
|
||||||
|
it 'refreshes the authorizations using a lease' do
|
||||||
|
expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).
|
||||||
|
and_return('foo')
|
||||||
|
|
||||||
|
expect(Gitlab::ExclusiveLease).to receive(:cancel).
|
||||||
|
with(an_instance_of(String), 'foo')
|
||||||
|
|
||||||
|
expect(service).to receive(:execute_without_lease)
|
||||||
|
|
||||||
|
service.execute
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe '#execute_without_lease' do
|
||||||
before do
|
before do
|
||||||
user.project_authorizations.delete_all
|
user.project_authorizations.delete_all
|
||||||
end
|
end
|
||||||
|
@ -19,37 +33,23 @@ describe Users::RefreshAuthorizedProjectsService do
|
||||||
project2 = create(:empty_project)
|
project2 = create(:empty_project)
|
||||||
to_remove = create_authorization(project2, user)
|
to_remove = create_authorization(project2, user)
|
||||||
|
|
||||||
expect(service).to receive(:update_with_lease).
|
expect(service).to receive(:update_authorizations).
|
||||||
with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
|
with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
|
||||||
|
|
||||||
service.execute
|
service.execute_without_lease
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'sets the access level of a project to the highest available level' do
|
it 'sets the access level of a project to the highest available level' do
|
||||||
to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER)
|
to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER)
|
||||||
|
|
||||||
expect(service).to receive(:update_with_lease).
|
expect(service).to receive(:update_authorizations).
|
||||||
with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
|
with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
|
||||||
|
|
||||||
service.execute
|
service.execute_without_lease
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'returns a User' do
|
it 'returns a User' do
|
||||||
expect(service.execute).to be_an_instance_of(User)
|
expect(service.execute_without_lease).to be_an_instance_of(User)
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
describe '#update_with_lease', :redis do
|
|
||||||
it 'refreshes the authorizations using a lease' do
|
|
||||||
expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).
|
|
||||||
and_return('foo')
|
|
||||||
|
|
||||||
expect(Gitlab::ExclusiveLease).to receive(:cancel).
|
|
||||||
with(an_instance_of(String), 'foo')
|
|
||||||
|
|
||||||
expect(service).to receive(:update_authorizations).with([1], [])
|
|
||||||
|
|
||||||
service.update_with_lease([1])
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue