Commit 5febecdf authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'ac/fix-use_file-race-ee' into 'master'

Add Gitlab::ExclusiveLease to ObjectStorage#use_file

See merge request gitlab-org/gitlab-ee!5158
parents 766a3e85 c6999f31
......@@ -228,17 +228,9 @@ module ObjectStorage
raise 'Failed to update object store' unless updated
end
def use_file
if file_storage?
return yield path
end
begin
cache_stored_file!
yield cache_path
ensure
FileUtils.rm_f(cache_path)
cache_storage.delete_dir!(cache_path(nil))
def use_file(&blk)
with_exclusive_lease do
unsafe_use_file(&blk)
end
end
......@@ -248,12 +240,9 @@ module ObjectStorage
# new_store: Enum (Store::LOCAL, Store::REMOTE)
#
def migrate!(new_store)
uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain
raise 'Already running' unless uuid
unsafe_migrate!(new_store)
ensure
Gitlab::ExclusiveLease.cancel(exclusive_lease_key, uuid)
with_exclusive_lease do
unsafe_migrate!(new_store)
end
end
def schedule_background_upload(*args)
......@@ -385,6 +374,15 @@ module ObjectStorage
"object_storage_migrate:#{model.class}:#{model.id}"
end
def with_exclusive_lease
uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain
raise 'exclusive lease already taken' unless uuid
yield uuid
ensure
Gitlab::ExclusiveLease.cancel(exclusive_lease_key, uuid)
end
#
# Move the file to another store
#
......@@ -419,4 +417,18 @@ module ObjectStorage
raise e
end
end
def unsafe_use_file
if file_storage?
return yield path
end
begin
cache_stored_file!
yield cache_path
ensure
FileUtils.rm_f(cache_path)
cache_storage.delete_dir!(cache_path(nil))
end
end
end
---
title: Fix data race between ObjectStorage background_upload and Pages publishing
merge_request:
author:
type: fixed
......@@ -61,12 +61,18 @@ shared_examples "migrates" do |to_store:, from_store: nil|
expect { migrate(to) }.not_to change { file.exists? }
end
context 'when migrate! is not oqqupied by another process' do
context 'when migrate! is not occupied by another process' do
it 'executes migrate!' do
expect(subject).to receive(:object_store=).at_least(1)
migrate(to)
end
it 'executes use_file' do
expect(subject).to receive(:unsafe_use_file).once
subject.use_file
end
end
context 'when migrate! is occupied by another process' do
......@@ -79,7 +85,13 @@ shared_examples "migrates" do |to_store:, from_store: nil|
it 'does not execute migrate!' do
expect(subject).not_to receive(:unsafe_migrate!)
expect { migrate(to) }.to raise_error('Already running')
expect { migrate(to) }.to raise_error('exclusive lease already taken')
end
it 'does not execute use_file' do
expect(subject).not_to receive(:unsafe_use_file)
expect { subject.use_file }.to raise_error('exclusive lease already taken')
end
after do
......
......@@ -320,6 +320,30 @@ describe ObjectStorage do
it { is_expected.to eq(remote_directory) }
end
context 'when file is in use' do
def when_file_is_in_use
uploader.use_file do
yield
end
end
it 'cannot migrate' do
when_file_is_in_use do
expect(uploader).not_to receive(:unsafe_migrate!)
expect { uploader.migrate!(described_class::Store::REMOTE) }.to raise_error('exclusive lease already taken')
end
end
it 'cannot use_file' do
when_file_is_in_use do
expect(uploader).not_to receive(:unsafe_use_file)
expect { uploader.use_file }.to raise_error('exclusive lease already taken')
end
end
end
describe '#fog_credentials' do
let(:connection) { Settingslogic.new("provider" => "AWS") }
......
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