Use upload ID instead of model ID in lease key

For FileUploaders it's possible that a model has many uploads
and if lease key is created only from model id, it causes that
the model's uploads can not be migrated in parallel because the
exclusive lease key would be same for all uploads of the model.
This commit is contained in:
Jan Provaznik 2018-06-08 22:06:08 +02:00
parent fa1a75ae28
commit c3f499e7c8
4 changed files with 28 additions and 8 deletions

View File

@ -298,6 +298,15 @@ module ObjectStorage
super
end
def exclusive_lease_key
# For FileUploaders, model may have many uploaders. In that case
# we want to use exclusive key per upload, not per model to allow
# parallel migration
key_object = self.is_a?(RecordsUploads::Concern) && upload ? upload : model
"object_storage_migrate:#{key_object.class}:#{key_object.id}"
end
private
def schedule_background_upload?
@ -364,10 +373,6 @@ module ObjectStorage
end
end
def exclusive_lease_key
"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

View File

@ -0,0 +1,5 @@
---
title: Use upload ID for creating lease key for file uploaders.
merge_request:
author:
type: fixed

View File

@ -76,10 +76,8 @@ shared_examples "migrates" do |to_store:, from_store: nil|
end
context 'when migrate! is occupied by another process' do
let(:exclusive_lease_key) { "object_storage_migrate:#{subject.model.class}:#{subject.model.id}" }
before do
@uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain
@uuid = Gitlab::ExclusiveLease.new(subject.exclusive_lease_key, timeout: 1.hour.to_i).try_obtain
end
it 'does not execute migrate!' do
@ -95,7 +93,7 @@ shared_examples "migrates" do |to_store:, from_store: nil|
end
after do
Gitlab::ExclusiveLease.cancel(exclusive_lease_key, @uuid)
Gitlab::ExclusiveLease.cancel(subject.exclusive_lease_key, @uuid)
end
end

View File

@ -332,6 +332,18 @@ describe ObjectStorage do
expect { uploader.use_file }.to raise_error('exclusive lease already taken')
end
end
it 'can still migrate other files of the same model' do
uploader2 = uploader_class.new(object, :file)
uploader2.upload = create(:upload)
uploader.upload = create(:upload)
when_file_is_in_use do
expect(uploader2).to receive(:unsafe_migrate!)
uploader2.migrate!(described_class::Store::REMOTE)
end
end
end
describe '#fog_credentials' do