diff options
author | Jan Provaznik <jprovaznik@gitlab.com> | 2018-06-08 23:06:08 +0300 |
---|---|---|
committer | Jan Provaznik <jprovaznik@gitlab.com> | 2018-06-12 10:34:07 +0300 |
commit | c3f499e7c8656fa09b333b88950c70f7377f95f9 (patch) | |
tree | 2d0a4175febeb24f5f613b72a4374b5a18131fef | |
parent | fa1a75ae28b421c5d3bd926c2795a053e4c6af15 (diff) |
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.
4 files changed, 28 insertions, 8 deletions
diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 23b3dcf84c0..d8ec5a81968 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -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 diff --git a/changelogs/unreleased/47513-upload-migration-lease-key-is-incorrect-for-non-mounted-uploaders.yml b/changelogs/unreleased/47513-upload-migration-lease-key-is-incorrect-for-non-mounted-uploaders.yml new file mode 100644 index 00000000000..010c4e9bce7 --- /dev/null +++ b/changelogs/unreleased/47513-upload-migration-lease-key-is-incorrect-for-non-mounted-uploaders.yml @@ -0,0 +1,5 @@ +--- +title: Use upload ID for creating lease key for file uploaders. +merge_request: +author: +type: fixed diff --git a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb index 6352f1527cd..12f7d3ed92a 100644 --- a/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb +++ b/spec/support/shared_examples/uploaders/object_storage_shared_examples.rb @@ -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 diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 0bc5b6751b3..902fc0bc030 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -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 |