Commit ae27a47b authored by Alessio Caiazza's avatar Alessio Caiazza

Add Gitlab::ExclusiveLease to ObjectStorage#use_file

parent cb94afc5
...@@ -228,16 +228,9 @@ module ObjectStorage ...@@ -228,16 +228,9 @@ module ObjectStorage
raise 'Failed to update object store' unless updated raise 'Failed to update object store' unless updated
end end
def use_file def use_file(&blk)
if file_storage? with_exclusive_lease do
return yield path unsafe_use_file(&blk)
end
begin
cache_stored_file!
yield cache_path
ensure
cache_storage.delete_dir!(cache_path(nil))
end end
end end
...@@ -247,12 +240,9 @@ module ObjectStorage ...@@ -247,12 +240,9 @@ module ObjectStorage
# new_store: Enum (Store::LOCAL, Store::REMOTE) # new_store: Enum (Store::LOCAL, Store::REMOTE)
# #
def migrate!(new_store) def migrate!(new_store)
uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain with_exclusive_lease do
raise 'Already running' unless uuid
unsafe_migrate!(new_store) unsafe_migrate!(new_store)
ensure end
Gitlab::ExclusiveLease.cancel(exclusive_lease_key, uuid)
end end
def schedule_background_upload(*args) def schedule_background_upload(*args)
...@@ -384,6 +374,15 @@ module ObjectStorage ...@@ -384,6 +374,15 @@ module ObjectStorage
"object_storage_migrate:#{model.class}:#{model.id}" "object_storage_migrate:#{model.class}:#{model.id}"
end 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 # Move the file to another store
# #
...@@ -418,4 +417,18 @@ module ObjectStorage ...@@ -418,4 +417,18 @@ module ObjectStorage
raise e raise e
end end
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 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| ...@@ -61,12 +61,18 @@ shared_examples "migrates" do |to_store:, from_store: nil|
expect { migrate(to) }.not_to change { file.exists? } expect { migrate(to) }.not_to change { file.exists? }
end 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 it 'executes migrate!' do
expect(subject).to receive(:object_store=).at_least(1) expect(subject).to receive(:object_store=).at_least(1)
migrate(to) migrate(to)
end end
it 'executes use_file' do
expect(subject).to receive(:unsafe_use_file).once
subject.use_file
end
end end
context 'when migrate! is occupied by another process' do context 'when migrate! is occupied by another process' do
...@@ -79,7 +85,13 @@ shared_examples "migrates" do |to_store:, from_store: nil| ...@@ -79,7 +85,13 @@ shared_examples "migrates" do |to_store:, from_store: nil|
it 'does not execute migrate!' do it 'does not execute migrate!' do
expect(subject).not_to receive(:unsafe_migrate!) 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 end
after do after do
......
...@@ -308,6 +308,30 @@ describe ObjectStorage do ...@@ -308,6 +308,30 @@ describe ObjectStorage do
it { is_expected.to eq(remote_directory) } it { is_expected.to eq(remote_directory) }
end 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 describe '#fog_credentials' do
let(:connection) { Settingslogic.new("provider" => "AWS") } 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