Refactor Geo::MoveRepositoryService

app/services/geo/hashed_storage_migration_service.rb

@@ -19,11 +19,10 @@ module Geo
     end
 
     def execute
-      project = Project.find(project_id)
       project.expire_caches_before_rename(old_disk_path)
 
-      if migrating_from_legacy_storage?(project)
-        Geo::MoveRepositoryService.new(project, old_disk_path, new_disk_path).execute
+      if migrating_from_legacy_storage? && !move_repository
+        raise RepositoryCannotBeRenamed, "Repository #{old_disk_path} could not be renamed to #{new_disk_path}"
       end
 
       true
@@ -31,12 +30,20 @@ module Geo
 
     private
 
-    def migrating_from_legacy_storage?(project)
+    def project
+      @project ||= Project.find(project_id)
+    end
+
+    def migrating_from_legacy_storage?
       from_legacy_storage? && project.hashed_storage?(:repository)
     end
 
     def from_legacy_storage?
       old_storage_version.nil? || old_storage_version.zero?
     end
+
+    def move_repository
+      Geo::MoveRepositoryService.new(project, old_disk_path, new_disk_path).execute
+    end
   end
 end

app/services/geo/move_repository_service.rb

 module Geo
+  RepositoryCannotBeRenamed = Class.new(StandardError)
+
   class MoveRepositoryService
     include Gitlab::ShellAdapter
+    include Gitlab::Geo::ProjectLogHelpers
 
     attr_reader :project, :old_disk_path, :new_disk_path
 
@@ -11,29 +14,21 @@ module Geo
     end
 
     def execute
-      # Make sure target directory exists (used when transfering repositories)
       project.ensure_storage_path_exists
-
-      if gitlab_shell.mv_repository(project.repository_storage_path,
-                                    old_disk_path, new_disk_path)
-        # If repository moved successfully we need to send update instructions to users.
-        # However we cannot allow rollback since we moved repository
-        # So we basically we mute exceptions in next actions
-        begin
-          gitlab_shell.mv_repository(project.repository_storage_path,
-                                     "#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
+      move_project_repository && move_wiki_repository
     rescue
-          # Returning false does not rollback after_* transaction but gives
-          # us information about failing some of tasks
+      log_error('Repository cannot be renamed')
       false
     end
-      else
-        # if we cannot move namespace directory we should rollback
-        # db changes in order to prevent out of sync between db and fs
-        raise StandardError.new('Repository cannot be renamed')
+
+    private
+
+    def move_project_repository
+      gitlab_shell.mv_repository(project.repository_storage_path, old_disk_path, new_disk_path)
     end
 
-      true
+    def move_wiki_repository
+      gitlab_shell.mv_repository(project.repository_storage_path, "#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
     end
   end
 end

app/services/geo/rename_repository_service.rb

@@ -13,11 +13,22 @@ module Geo
     end
 
     def execute
-      project = Project.find(project_id)
       project.expire_caches_before_rename(old_disk_path)
 
-      return true if project.hashed_storage?(:repository)
+      if project.legacy_storage? && !move_repository
+        raise RepositoryCannotBeRenamed, "Repository #{old_disk_path} could not be renamed to #{new_disk_path}"
+      end
+
+      true
+    end
+
+    private
+
+    def project
+      @project ||= Project.find(project_id)
+    end
 
+    def move_repository
       Geo::MoveRepositoryService.new(project, old_disk_path, new_disk_path).execute
     end
   end

changelogs/unreleased-ee/refactor-geo-move-repository-service.yml

+---
+title: Geo - Allow Sidekiq to retry failed jobs to rename project repositories
+merge_request:
+author:
+type: fixed

spec/services/geo/hashed_storage_migration_service_spec.rb

@@ -2,22 +2,41 @@ require 'spec_helper'
 
 describe Geo::HashedStorageMigrationService do
   let(:project) { create(:project, :repository, :hashed) }
-  let(:new_path) { "#{project.full_path}+renamed" }
+  let(:old_path) { project.full_path }
+  let(:new_path) { "#{old_path}+renamed" }
 
-  describe '#execute' do
-    it 'moves project backed by legacy storage' do
-      service = described_class.new(
-        project.id,
-        old_disk_path: project.full_path,
-        new_disk_path: new_path,
-        old_storage_version: nil
-      )
+  subject(:service) { described_class.new(project.id, old_disk_path: old_path, new_disk_path: new_path, old_storage_version: nil) }
 
-      expect_any_instance_of(Geo::MoveRepositoryService).to receive(:execute).once
+  describe '#execute' do
+    context 'project backed by legacy storage' do
+      it 'moves the project repositories' do
+        expect_any_instance_of(Geo::MoveRepositoryService).to receive(:execute)
+          .once.and_return(true)
 
         service.execute
       end
 
+      it 'raises an error when project repository can not be moved' do
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage_path, old_path, new_path)
+          .and_return(false)
+
+        expect { service.execute }.to raise_error(Geo::RepositoryCannotBeRenamed, "Repository #{old_path} could not be renamed to #{new_path}")
+      end
+
+      it 'raises an error when wiki repository can not be moved' do
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage_path, old_path, new_path)
+          .and_return(true)
+
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage_path, "#{old_path}.wiki", "#{new_path}.wiki")
+          .and_return(false)
+
+        expect { service.execute }.to raise_error(Geo::RepositoryCannotBeRenamed, "Repository #{old_path} could not be renamed to #{new_path}")
+      end
+    end
+
     it 'does not move project backed by hashed storage' do
       service = described_class.new(
         project.id,
@@ -33,8 +52,6 @@ describe Geo::HashedStorageMigrationService do
   end
 
   describe '#async_execute' do
-    subject(:service) { described_class.new(project.id, old_disk_path: project.full_path, new_disk_path: new_path, old_storage_version: nil) }
-
     it 'starts the worker' do
       expect(Geo::HashedStorageMigrationWorker).to receive(:perform_async)
 

spec/services/geo/move_repository_service_spec.rb

 require 'spec_helper'
 
 describe Geo::MoveRepositoryService do
-  let(:project) { create(:project, :repository) }
+  describe '#execute' do
+    let(:project) { create(:project, :repository, :wiki_repo) }
+    let(:old_path) { project.full_path }
     let(:new_path) { "#{project.full_path}+renamed" }
 
-  describe '#execute' do
-    it 'renames the path' do
-      old_path = project.repository.path_to_repo
-      full_new_path = File.join(project.repository_storage_path, new_path)
+    subject(:service) { described_class.new(project, old_path, new_path) }
 
-      service = described_class.new(project, project.full_path, new_path)
+    it 'renames the project repositories' do
+      old_disk_path = project.repository.path_to_repo
+      old_wiki_disk_path = project.wiki.repository.path_to_repo
+      full_new_path = File.join(project.repository_storage_path, new_path)
 
-      expect(File.directory?(old_path)).to be_truthy
+      expect(File.directory?(old_disk_path)).to be_truthy
+      expect(File.directory?(old_wiki_disk_path)).to be_truthy
       expect(service.execute).to eq(true)
-      expect(File.directory?(old_path)).to be_falsey
+      expect(File.directory?(old_disk_path)).to be_falsey
+      expect(File.directory?(old_wiki_disk_path)).to be_falsey
       expect(File.directory?("#{full_new_path}.git")).to be_truthy
+      expect(File.directory?("#{full_new_path}.wiki.git")).to be_truthy
+    end
+
+    it 'returns false when project repository can not be renamed' do
+      allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+        .with(project.repository_storage_path, old_path, new_path)
+        .and_return(false)
+
+      expect(service.execute).to eq false
+    end
+
+    it 'returns false when wiki repository can not be renamed' do
+      allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+        .with(project.repository_storage_path, old_path, new_path)
+        .and_return(true)
+
+      allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+        .with(project.repository_storage_path, "#{old_path}.wiki", "#{new_path}.wiki")
+        .and_return(false)
+
+      expect(service.execute).to eq false
     end
   end
 end

spec/services/geo/rename_repository_service_spec.rb

@@ -2,17 +2,41 @@ require 'spec_helper'
 
 describe Geo::RenameRepositoryService do
   let(:project) { create(:project, :repository) }
-  let(:new_path) { "#{project.full_path}+renamed" }
+  let(:old_path) { project.full_path }
+  let(:new_path) { "#{old_path}+renamed" }
 
-  describe '#execute' do
-    it 'moves project backed by legacy storage' do
-      service = described_class.new(project.id, project.full_path, new_path)
+  subject(:service) { described_class.new(project.id, old_path, new_path) }
 
-      expect_any_instance_of(Geo::MoveRepositoryService).to receive(:execute).once
+  describe '#execute' do
+    context 'project backed by legacy storage' do
+      it 'moves the project repositories' do
+        expect_any_instance_of(Geo::MoveRepositoryService).to receive(:execute)
+          .once.and_return(true)
 
         service.execute
       end
 
+      it 'raises an error when project repository can not be moved' do
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage_path, old_path, new_path)
+          .and_return(false)
+
+        expect { service.execute }.to raise_error(Geo::RepositoryCannotBeRenamed, "Repository #{old_path} could not be renamed to #{new_path}")
+      end
+
+      it 'raises an error when wiki repository can not be moved' do
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage_path, old_path, new_path)
+          .and_return(true)
+
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage_path, "#{old_path}.wiki", "#{new_path}.wiki")
+          .and_return(false)
+
+        expect { service.execute }.to raise_error(Geo::RepositoryCannotBeRenamed, "Repository #{old_path} could not be renamed to #{new_path}")
+      end
+    end
+
     it 'does not move project backed by hashed storage' do
       project_hashed_storage = create(:project, :hashed)
       service = described_class.new(project_hashed_storage.id, project_hashed_storage.full_path, new_path)
@@ -24,8 +48,6 @@ describe Geo::RenameRepositoryService do
   end
 
   describe '#async_execute' do
-    subject(:service) { described_class.new(project.id, project.full_path, new_path) }
-
     it 'starts the worker' do
       expect(Geo::RenameRepositoryWorker).to receive(:perform_async)