Merge branch 'jv-repository-storage-idempotent' into 'master'

Make UpdateRepositoryStorageService idempotent

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

ee/app/services/projects/update_repository_storage_service.rb

@@ -4,11 +4,19 @@ module Projects
   class UpdateRepositoryStorageService < BaseService
     include Gitlab::ShellAdapter
 
+    RepositoryAlreadyMoved = Class.new(StandardError)
+
     def initialize(project)
       @project = project
     end
 
     def execute(new_repository_storage_key)
+      # Raising an exception is a little heavy handed but this behavior (doing
+      # nothing if the repo is already on the right storage) prevents data
+      # loss, so it is valuable for us to be able to observe it via the
+      # exception.
+      raise RepositoryAlreadyMoved if project.repository_storage == new_repository_storage_key
+
       result = mirror_repository(new_repository_storage_key)
 
       if project.wiki.repository_exists?

ee/app/workers/project_update_repository_storage_worker.rb

@@ -7,5 +7,7 @@ class ProjectUpdateRepositoryStorageWorker
     project = Project.find(project_id)
 
     ::Projects::UpdateRepositoryStorageService.new(project).execute(new_repository_storage_key)
+  rescue ::Projects::UpdateRepositoryStorageService::RepositoryAlreadyMoved
+    Rails.logger.info "#{self.class}: repository already moved: #{project}"
   end
 end

ee/changelogs/unreleased/jv-repository-storage-idempotent.yml

+---
+title: Make UpdateRepositoryStorageService idempotent
+merge_request: 10457
+author:
+type: fixed

ee/spec/services/projects/update_repository_storage_service_spec.rb

@@ -32,6 +32,14 @@ describe Projects::UpdateRepositoryStorageService do
         end
       end
 
+      context 'when the project is already on the target storage' do
+        it 'bails out and does nothing' do
+          expect do
+            subject.execute(project.repository_storage)
+          end.to raise_error(described_class::RepositoryAlreadyMoved)
+        end
+      end
+
       context 'when the move fails' do
         it 'unmarks the repository as read-only without updating the repository storage' do
           expect_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror)
@@ -90,6 +98,14 @@ describe Projects::UpdateRepositoryStorageService do
         end
       end
 
+      context 'when the project is already on the target storage' do
+        it 'bails out and does nothing' do
+          expect do
+            subject.execute(project.repository_storage)
+          end.to raise_error(described_class::RepositoryAlreadyMoved)
+        end
+      end
+
       context 'when the move of the wiki fails' do
         it 'unmarks the repository as read-only without updating the repository storage' do
           expect(repository_double).to receive(:fetch_repository_as_mirror)

ee/spec/workers/project_update_repository_storage_worker_spec.rb

@@ -12,5 +12,11 @@ describe ProjectUpdateRepositoryStorageWorker do
 
       subject.perform(project.id, 'new_storage')
     end
+
+    it 'catches and logs RepositoryAlreadyMoved' do
+      expect(Rails.logger).to receive(:info).with(/repository already moved/)
+
+      expect { subject.perform(project.id, project.repository_storage) }.not_to raise_error
+    end
   end
 end