diff options
| -rw-r--r-- | app/uploaders/object_storage.rb | 78 | ||||
| -rw-r--r-- | spec/models/lfs_object_spec.rb | 110 | ||||
| -rw-r--r-- | spec/requests/lfs_http_spec.rb | 2 |
3 files changed, 162 insertions, 28 deletions
diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 43d881e5aba..1880cd100dc 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -88,7 +88,13 @@ module ObjectStorage def changed_mounts self.class.uploaders.select do |mount, uploader_class| mounted_as = uploader_class.serialization_column(self.class, mount) - mount if send(:"#{mounted_as}_changed?") # rubocop:disable GitlabSecurity/PublicSend + uploader = send(:"#{mounted_as}") # rubocop:disable GitlabSecurity/PublicSend + + next unless uploader + next unless uploader.exists? + next unless send(:"#{mounted_as}_changed?") # rubocop:disable GitlabSecurity/PublicSend + + mount end.keys end @@ -164,7 +170,7 @@ module ObjectStorage return unless persist_object_store? updated = model.update_column(store_serialization_column, object_store) - raise ActiveRecordError unless updated + raise 'Failed to update object store' unless updated end def use_file @@ -190,32 +196,12 @@ module ObjectStorage # new_store: Enum (Store::LOCAL, Store::REMOTE) # def migrate!(new_store) - return unless object_store != new_store - return unless file - - new_file = nil - file_to_delete = file - from_object_store = object_store - self.object_store = new_store # changes the storage and file - - cache_stored_file! if file_storage? + uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain + raise 'Already running' unless uuid - with_callbacks(:migrate, file_to_delete) do - with_callbacks(:store, file_to_delete) do # for #store_versions! - new_file = storage.store!(file) - persist_object_store! - self.file = new_file - end - end - - file - rescue => e - # in case of failure delete new file - new_file.delete unless new_file.nil? - # revert back to the old file - self.object_store = from_object_store - self.file = file_to_delete - raise e + unsafe_migrate!(new_store) + ensure + Gitlab::ExclusiveLease.cancel(exclusive_lease_key, uuid) end def schedule_background_upload(*args) @@ -298,5 +284,43 @@ module ObjectStorage raise UnknownStoreError end end + + def exclusive_lease_key + "object_storage_migrate:#{model.class}:#{model.id}" + end + + # + # Move the file to another store + # + # new_store: Enum (Store::LOCAL, Store::REMOTE) + # + def unsafe_migrate!(new_store) + return unless object_store != new_store + return unless file + + new_file = nil + file_to_delete = file + from_object_store = object_store + self.object_store = new_store # changes the storage and file + + cache_stored_file! if file_storage? + + with_callbacks(:migrate, file_to_delete) do + with_callbacks(:store, file_to_delete) do # for #store_versions! + new_file = storage.store!(file) + persist_object_store! + self.file = new_file + end + end + + file + rescue => e + # in case of failure delete new file + new_file.delete unless new_file.nil? + # revert back to the old file + self.object_store = from_object_store + self.file = file_to_delete + raise e + end end end diff --git a/spec/models/lfs_object_spec.rb b/spec/models/lfs_object_spec.rb new file mode 100644 index 00000000000..87f4daab9be --- /dev/null +++ b/spec/models/lfs_object_spec.rb @@ -0,0 +1,110 @@ +require 'spec_helper' + +describe LfsObject do + describe '#local_store?' do + it 'returns true when file_store is nil' do + subject.file_store = nil + + expect(subject.local_store?).to eq true + end + + it 'returns true when file_store is equal to LfsObjectUploader::Store::LOCAL' do + subject.file_store = LfsObjectUploader::Store::LOCAL + + expect(subject.local_store?).to eq true + end + + it 'returns false whe file_store is equal to LfsObjectUploader::Store::REMOTE' do + subject.file_store = LfsObjectUploader::Store::REMOTE + + expect(subject.local_store?).to eq false + end + end + + describe '#destroy' do + subject { create(:lfs_object, :with_file) } + + context 'when running in a Geo primary node' do + set(:primary) { create(:geo_node, :primary) } + set(:secondary) { create(:geo_node) } + + it 'logs an event to the Geo event log' do + expect { subject.destroy }.to change(Geo::LfsObjectDeletedEvent, :count).by(1) + end + end + end + + describe '#schedule_background_upload' do + before do + stub_lfs_setting(enabled: true) + end + + subject { create(:lfs_object, :with_file) } + + context 'when object storage is disabled' do + before do + stub_lfs_object_storage(enabled: false) + end + + it 'does not schedule the migration' do + expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) + + subject + end + end + + context 'when object storage is enabled' do + context 'when background upload is enabled' do + context 'when is licensed' do + before do + stub_lfs_object_storage(background_upload: true) + end + + it 'schedules the model for migration' do + expect(ObjectStorage::BackgroundMoveWorker) + .to receive(:perform_async) + .with('LfsObjectUploader', described_class.name, :file, kind_of(Numeric)) + .once + + subject + end + + it 'schedules the model for migration once' do + expect(ObjectStorage::BackgroundMoveWorker) + .to receive(:perform_async) + .with('LfsObjectUploader', described_class.name, :file, kind_of(Numeric)) + .once + + lfs_object = create(:lfs_object) + lfs_object.file = fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png") + lfs_object.save! + end + end + + context 'when is unlicensed' do + before do + stub_lfs_object_storage(background_upload: true, licensed: false) + end + + it 'does not schedule the migration' do + expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) + + subject + end + end + end + + context 'when background upload is disabled' do + before do + stub_lfs_object_storage(background_upload: false) + end + + it 'schedules the model for migration' do + expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) + + subject + end + end + end + end +end diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 0ddbddb112c..f7c04c19903 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -997,7 +997,7 @@ describe 'Git LFS API and storage' do context 'and workhorse requests upload finalize for a new lfs object' do before do - allow_any_instance_of(LfsObjectUploader).to receive(:exists?) { false } + lfs_object.destroy end context 'with object storage disabled' do |