Merge branch 'fix/sm/atomic-migration' into 'master'

Fix migrate! method (Minimal fix with ExclusiveLock to prevent race conditions)

Closes #4928 and #4980

See merge request gitlab-org/gitlab-ee!4624

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

spec/models/lfs_object_spec.rb

+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

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