Commit 0e4147b4 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'fix_concurrent_backup' into 'master'

Fix deadlock in backup repositories rake task

Closes #241701

See merge request gitlab-org/gitlab!41042
parents 57d4dbf9 2ee51e06
---
title: Fix deadlock in backup repositories rake task
merge_request: 41042
author:
type: fixed
...@@ -148,20 +148,22 @@ module Backup ...@@ -148,20 +148,22 @@ module Backup
private private
def dump_consecutive def dump_consecutive
Project.find_each(batch_size: 1000) do |project| Project.includes(:route).find_each(batch_size: 1000) do |project|
dump_project(project) dump_project(project)
end end
end end
def dump_storage(storage, semaphore, max_storage_concurrency:) def dump_storage(storage, semaphore, max_storage_concurrency:)
errors = Queue.new errors = Queue.new
queue = SizedQueue.new(1) queue = InterlockSizedQueue.new(1)
threads = Array.new(max_storage_concurrency) do threads = Array.new(max_storage_concurrency) do
Thread.new do Thread.new do
Rails.application.executor.wrap do Rails.application.executor.wrap do
while project = queue.pop while project = queue.pop
ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
semaphore.acquire semaphore.acquire
end
begin begin
dump_project(project) dump_project(project)
...@@ -176,7 +178,7 @@ module Backup ...@@ -176,7 +178,7 @@ module Backup
end end
end end
Project.for_repository_storage(storage).find_each(batch_size: 100) do |project| Project.for_repository_storage(storage).includes(:route).find_each(batch_size: 100) do |project|
break unless errors.empty? break unless errors.empty?
queue.push(project) queue.push(project)
...@@ -241,5 +243,23 @@ module Backup ...@@ -241,5 +243,23 @@ module Backup
pool.schedule pool.schedule
end end
end end
class InterlockSizedQueue < SizedQueue
extend ::Gitlab::Utils::Override
override :pop
def pop(*)
ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
super
end
end
override :push
def push(*)
ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
super
end
end
end
end end
end end
...@@ -47,15 +47,27 @@ RSpec.describe Backup::Repository do ...@@ -47,15 +47,27 @@ RSpec.describe Backup::Repository do
end end
it 'project query raises an error' do it 'project query raises an error' do
allow(Project).to receive(:find_each).and_raise(ActiveRecord::StatementTimeout) allow(Project).to receive_message_chain(:includes, :find_each).and_raise(ActiveRecord::StatementTimeout)
expect { subject.dump(max_concurrency: 1, max_storage_concurrency: 1) }.to raise_error(ActiveRecord::StatementTimeout) expect { subject.dump(max_concurrency: 1, max_storage_concurrency: 1) }.to raise_error(ActiveRecord::StatementTimeout)
end end
end end
it 'avoids N+1 database queries' do
control_count = ActiveRecord::QueryRecorder.new do
subject.dump(max_concurrency: 1, max_storage_concurrency: 1)
end.count
create_list(:project, 2, :wiki_repo)
expect do
subject.dump(max_concurrency: 1, max_storage_concurrency: 1)
end.not_to exceed_query_limit(control_count)
end
end end
[4, 10].each do |max_storage_concurrency| [4, 10].each do |max_storage_concurrency|
context "max_storage_concurrency #{max_storage_concurrency}", quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/241701' do context "max_storage_concurrency #{max_storage_concurrency}" do
it 'creates the expected number of threads' do it 'creates the expected number of threads' do
expect(Thread).to receive(:new) expect(Thread).to receive(:new)
.exactly(storage_keys.length * (max_storage_concurrency + 1)).times .exactly(storage_keys.length * (max_storage_concurrency + 1)).times
...@@ -89,7 +101,7 @@ RSpec.describe Backup::Repository do ...@@ -89,7 +101,7 @@ RSpec.describe Backup::Repository do
end end
it 'project query raises an error' do it 'project query raises an error' do
allow(Project).to receive_message_chain('for_repository_storage.find_each').and_raise(ActiveRecord::StatementTimeout) allow(Project).to receive_message_chain(:for_repository_storage, :includes, :find_each).and_raise(ActiveRecord::StatementTimeout)
expect { subject.dump(max_concurrency: 1, max_storage_concurrency: max_storage_concurrency) }.to raise_error(ActiveRecord::StatementTimeout) expect { subject.dump(max_concurrency: 1, max_storage_concurrency: max_storage_concurrency) }.to raise_error(ActiveRecord::StatementTimeout)
end end
...@@ -102,6 +114,18 @@ RSpec.describe Backup::Repository do ...@@ -102,6 +114,18 @@ RSpec.describe Backup::Repository do
end end
end end
end end
it 'avoids N+1 database queries' do
control_count = ActiveRecord::QueryRecorder.new do
subject.dump(max_concurrency: 1, max_storage_concurrency: max_storage_concurrency)
end.count
create_list(:project, 2, :wiki_repo)
expect do
subject.dump(max_concurrency: 1, max_storage_concurrency: max_storage_concurrency)
end.not_to exceed_query_limit(control_count)
end
end 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