Try to make repository storage change failures informative

Adds proper service success/error calls and error tracking

app/services/projects/update_repository_storage_service.rb

@@ -4,6 +4,7 @@ module Projects
   class UpdateRepositoryStorageService < BaseService
     include Gitlab::ShellAdapter
 
+    Error = Class.new(StandardError)
     RepositoryAlreadyMoved = Class.new(StandardError)
 
     def initialize(project)
@@ -17,33 +18,40 @@ module Projects
       # exception.
       raise RepositoryAlreadyMoved if project.repository_storage == new_repository_storage_key
 
-      if mirror_repositories(new_repository_storage_key)
-        mark_old_paths_for_archive
+      mirror_repositories(new_repository_storage_key)
 
-        project.update(repository_storage: new_repository_storage_key, repository_read_only: false)
-        project.leave_pool_repository
-        project.track_project_repository
+      mark_old_paths_for_archive
 
-        enqueue_housekeeping
-      else
-        project.update(repository_read_only: false)
-      end
+      project.update(repository_storage: new_repository_storage_key, repository_read_only: false)
+      project.leave_pool_repository
+      project.track_project_repository
+
+      enqueue_housekeeping
+
+      success
+
+    rescue Error => e
+      project.update(repository_read_only: false)
+
+      Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path)
+
+      error(s_("UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}") % { project_full_path: project.full_path, message: e.message })
     end
 
     private
 
     def mirror_repositories(new_repository_storage_key)
-      result = mirror_repository(new_repository_storage_key)
+      mirror_repository(new_repository_storage_key)
 
       if project.wiki.repository_exists?
-        result &&= mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::WIKI)
+        mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::WIKI)
       end
-
-      result
     end
 
     def mirror_repository(new_storage_key, type: Gitlab::GlRepository::PROJECT)
-      return false unless wait_for_pushes(type)
+      unless wait_for_pushes(type)
+        raise Error, s_('UpdateRepositoryStorage|Timeout waiting for %{type} repository pushes') % { type: type.name }
+      end
 
       repository = type.repository_for(project)
       full_path = repository.full_path
@@ -57,9 +65,15 @@ module Projects
                                                    raw_repository.gl_repository,
                                                    full_path)
 
-      result = new_repository.fetch_repository_as_mirror(raw_repository)
+      unless new_repository.fetch_repository_as_mirror(raw_repository)
+        raise Error, s_('UpdateRepositoryStorage|Failed to fetch %{type} repository as mirror') % { type: type.name }
+      end
 
-      result && checksum == new_repository.checksum
+      new_checksum = new_repository.checksum
+
+      if checksum != new_checksum
+        raise Error, s_('UpdateRepositoryStorage|Failed to verify %{type} repository checksum from %{old} to %{new}') % { type: type.name, old: checksum, new: new_checksum }
+      end
     end
 
     def mark_old_paths_for_archive

ee/app/services/ee/projects/update_repository_storage_service.rb

@@ -7,13 +7,11 @@ module EE
 
       override :mirror_repositories
       def mirror_repositories(new_repository_storage_key)
-        result = super
+        super
 
         if project.design_repository.exists?
-          result &&= mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::DESIGN)
+          mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::DESIGN)
         end
-
-        result
       end
 
       override :mark_old_paths_for_archive

locale/gitlab.pot

@@ -21215,6 +21215,18 @@ msgstr ""
 msgid "UpdateProject|Project could not be updated!"
 msgstr ""
 
+msgid "UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}"
+msgstr ""
+
+msgid "UpdateRepositoryStorage|Failed to fetch %{type} repository as mirror"
+msgstr ""
+
+msgid "UpdateRepositoryStorage|Failed to verify %{type} repository checksum from %{old} to %{new}"
+msgstr ""
+
+msgid "UpdateRepositoryStorage|Timeout waiting for %{type} repository pushes"
+msgstr ""
+
 msgid "Updated"
 msgstr ""
 

spec/services/projects/update_repository_storage_service_spec.rb

@@ -37,7 +37,9 @@ describe Projects::UpdateRepositoryStorageService do
           expect(project_repository_double).to receive(:checksum)
             .and_return(checksum)
 
-          subject.execute('test_second_storage')
+          result = subject.execute('test_second_storage')
+
+          expect(result[:status]).to eq(:success)
           expect(project).not_to be_repository_read_only
           expect(project.repository_storage).to eq('test_second_storage')
           expect(gitlab_shell.repository_exists?('default', old_path)).to be(false)
@@ -59,8 +61,9 @@ describe Projects::UpdateRepositoryStorageService do
             .with(project.repository.raw).and_return(false)
           expect(GitlabShellWorker).not_to receive(:perform_async)
 
-          subject.execute('test_second_storage')
+          result = subject.execute('test_second_storage')
 
+          expect(result[:status]).to eq(:error)
           expect(project).not_to be_repository_read_only
           expect(project.repository_storage).to eq('default')
         end
@@ -74,8 +77,9 @@ describe Projects::UpdateRepositoryStorageService do
             .and_return('not matching checksum')
           expect(GitlabShellWorker).not_to receive(:perform_async)
 
-          subject.execute('test_second_storage')
+          result = subject.execute('test_second_storage')
 
+          expect(result[:status]).to eq(:error)
           expect(project).not_to be_repository_read_only
           expect(project.repository_storage).to eq('default')
         end
@@ -90,8 +94,9 @@ describe Projects::UpdateRepositoryStorageService do
           expect(project_repository_double).to receive(:checksum)
             .and_return(checksum)
 
-          subject.execute('test_second_storage')
+          result = subject.execute('test_second_storage')
 
+          expect(result[:status]).to eq(:success)
           expect(project.repository_storage).to eq('test_second_storage')
           expect(project.reload_pool_repository).to be_nil
         end

spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb

@@ -41,8 +41,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
       old_repository_path = repository.full_path
 
-      subject.execute('test_second_storage')
+      result = subject.execute('test_second_storage')
 
+      expect(result[:status]).to eq(:success)
       expect(project).not_to be_repository_read_only
       expect(project.repository_storage).to eq('test_second_storage')
       expect(gitlab_shell.repository_exists?('default', old_project_repository_path)).to be(false)
@@ -98,8 +99,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
       expect(GitlabShellWorker).not_to receive(:perform_async)
 
-      subject.execute('test_second_storage')
+      result = subject.execute('test_second_storage')
 
+      expect(result[:status]).to eq(:error)
       expect(project).not_to be_repository_read_only
       expect(project.repository_storage).to eq('default')
     end
@@ -119,8 +121,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
       expect(GitlabShellWorker).not_to receive(:perform_async)
 
-      subject.execute('test_second_storage')
+      result = subject.execute('test_second_storage')
 
+      expect(result[:status]).to eq(:error)
       expect(project).not_to be_repository_read_only
       expect(project.repository_storage).to eq('default')
     end