Remove repositories from previous storage when storage move succeeds

Cleanup states were added to keep track of this removal as a failure to
remove will require manual intervention from an admin.

app/models/project_repository_storage_move.rb

@@ -29,12 +29,17 @@ class ProjectRepositoryStorageMove < ApplicationRecord
       transition scheduled: :started
     end
 
-    event :finish do
-      transition started: :finished
+    event :finish_replication do
+      transition started: :replicated
+    end
+
+    event :finish_cleanup do
+      transition replicated: :finished
     end
 
     event :do_fail do
       transition [:initial, :scheduled, :started] => :failed
+      transition replicated: :cleanup_failed
     end
 
     after_transition initial: :scheduled do |storage_move|
@@ -49,7 +54,7 @@ class ProjectRepositoryStorageMove < ApplicationRecord
       end
     end
 
-    after_transition started: :finished do |storage_move|
+    after_transition started: :replicated do |storage_move|
       storage_move.project.update_columns(
         repository_read_only: false,
         repository_storage: storage_move.destination_storage_name
@@ -65,6 +70,8 @@ class ProjectRepositoryStorageMove < ApplicationRecord
     state :started, value: 3
     state :finished, value: 4
     state :failed, value: 5
+    state :replicated, value: 6
+    state :cleanup_failed, value: 7
   end
 
   scope :order_created_at_desc, -> { order(created_at: :desc) }

app/services/projects/update_repository_storage_service.rb

@@ -6,8 +6,7 @@ module Projects
     SameFilesystemError = Class.new(Error)
 
     attr_reader :repository_storage_move
-    delegate :project, :destination_storage_name, to: :repository_storage_move
-    delegate :repository, to: :project
+    delegate :project, :source_storage_name, :destination_storage_name, to: :repository_storage_move
 
     def initialize(repository_storage_move)
       @repository_storage_move = repository_storage_move
@@ -20,21 +19,22 @@ module Projects
         repository_storage_move.start!
       end
 
-      raise SameFilesystemError if same_filesystem?(repository.storage, destination_storage_name)
+      raise SameFilesystemError if same_filesystem?(source_storage_name, destination_storage_name)
 
       mirror_repositories
 
-      project.transaction do
-        mark_old_paths_for_archive
-
-        repository_storage_move.finish!
+      repository_storage_move.transaction do
+        repository_storage_move.finish_replication!
 
         project.leave_pool_repository
         project.track_project_repository
       end
 
+      remove_old_paths
       enqueue_housekeeping
 
+      repository_storage_move.finish_cleanup!
+
       ServiceResponse.success
 
     rescue StandardError => e
@@ -91,36 +91,31 @@ module Projects
       end
     end
 
-    def mark_old_paths_for_archive
-      old_repository_storage = project.repository_storage
-      new_project_path = moved_path(project.disk_path)
-
-      # Notice that the block passed to `run_after_commit` will run with `repository_storage_move`
-      # as its context
-      repository_storage_move.run_after_commit do
-        GitlabShellWorker.perform_async(:mv_repository,
-                                        old_repository_storage,
-                                        project.disk_path,
-                                        new_project_path)
-
-        if project.wiki.repository_exists?
-          GitlabShellWorker.perform_async(:mv_repository,
-                                          old_repository_storage,
-                                          project.wiki.disk_path,
-                                          "#{new_project_path}.wiki")
-        end
-
-        if project.design_repository.exists?
-          GitlabShellWorker.perform_async(:mv_repository,
-                                          old_repository_storage,
-                                          project.design_repository.disk_path,
-                                          "#{new_project_path}.design")
-        end
+    def remove_old_paths
+      Gitlab::Git::Repository.new(
+        source_storage_name,
+        "#{project.disk_path}.git",
+        nil,
+        nil
+      ).remove
+
+      if project.wiki.repository_exists?
+        Gitlab::Git::Repository.new(
+          source_storage_name,
+          "#{project.wiki.disk_path}.git",
+          nil,
+          nil
+        ).remove
       end
-    end
 
-    def moved_path(path)
-      "#{path}+#{project.id}+moved+#{Time.current.to_i}"
+      if project.design_repository.exists?
+        Gitlab::Git::Repository.new(
+          source_storage_name,
+          "#{project.design_repository.disk_path}.git",
+          nil,
+          nil
+        ).remove
+      end
     end
 
     # The underlying FetchInternalRemote call uses a `git fetch` to move data

changelogs/unreleased/storage_move_cleanup.yml

+---
+title: Remove repositories from previous storage when storage move succeeds
+merge_request: 38547
+author:
+type: changed

spec/factories/project_repository_storage_moves.rb

@@ -15,6 +15,10 @@ FactoryBot.define do
       state { ProjectRepositoryStorageMove.state_machines[:state].states[:started].value }
     end
 
+    trait :replicated do
+      state { ProjectRepositoryStorageMove.state_machines[:state].states[:replicated].value }
+    end
+
     trait :finished do
       state { ProjectRepositoryStorageMove.state_machines[:state].states[:finished].value }
     end

spec/models/project_repository_storage_move_spec.rb

@@ -74,9 +74,9 @@ RSpec.describe ProjectRepositoryStorageMove, type: :model do
     context 'when started' do
       subject(:storage_move) { create(:project_repository_storage_move, :started, project: project, destination_storage_name: 'test_second_storage') }
 
-      context 'and transits to finished' do
+      context 'and transits to replicated' do
         it 'sets the repository storage and marks the project as writable' do
-          storage_move.finish!
+          storage_move.finish_replication!
 
           expect(project.repository_storage).to eq('test_second_storage')
           expect(project).not_to be_repository_read_only

spec/services/projects/update_repository_storage_service_spec.rb

@@ -21,6 +21,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
       let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) }
       let!(:checksum) { project.repository.checksum }
       let(:project_repository_double) { double(:repository) }
+      let(:original_project_repository_double) { double(:repository) }
 
       before do
         allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
@@ -29,6 +30,9 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
         allow(Gitlab::Git::Repository).to receive(:new)
           .with('test_second_storage', project.repository.raw.relative_path, project.repository.gl_repository, project.repository.full_path)
           .and_return(project_repository_double)
+        allow(Gitlab::Git::Repository).to receive(:new)
+          .with('default', project.repository.raw.relative_path, nil, nil)
+          .and_return(original_project_repository_double)
       end
 
       context 'when the move succeeds' do
@@ -41,8 +45,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
             .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
             .and_return(checksum)
-          expect(GitlabShellWorker).to receive(:perform_async).with(:mv_repository, 'default', anything, anything)
-            .and_call_original
+          expect(original_project_repository_double).to receive(:remove)
 
           result = subject.execute
           project.reload
@@ -74,13 +77,29 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
           expect(project_repository_double).to receive(:replicate)
             .with(project.repository.raw)
             .and_raise(Gitlab::Git::CommandError)
-          expect(GitlabShellWorker).not_to receive(:perform_async)
 
           result = subject.execute
 
           expect(result).to be_error
           expect(project).not_to be_repository_read_only
           expect(project.repository_storage).to eq('default')
+          expect(repository_storage_move).to be_failed
+        end
+      end
+
+      context 'when the cleanup fails' do
+        it 'sets the correct state' do
+          expect(project_repository_double).to receive(:replicate)
+            .with(project.repository.raw)
+          expect(project_repository_double).to receive(:checksum)
+            .and_return(checksum)
+          expect(original_project_repository_double).to receive(:remove)
+            .and_raise(Gitlab::Git::CommandError)
+
+          result = subject.execute
+
+          expect(result).to be_error
+          expect(repository_storage_move).to be_cleanup_failed
         end
       end
 
@@ -93,7 +112,6 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
             .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
             .and_return('not matching checksum')
-          expect(GitlabShellWorker).not_to receive(:perform_async)
 
           result = subject.execute
 
@@ -114,6 +132,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do
             .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
             .and_return(checksum)
+          expect(original_project_repository_double).to receive(:remove)
 
           result = subject.execute
           project.reload

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

@@ -2,9 +2,11 @@
 
 RSpec.shared_examples 'moves repository to another storage' do |repository_type|
   let(:project_repository_double) { double(:repository) }
+  let(:original_project_repository_double) { double(:repository) }
   let!(:project_repository_checksum) { project.repository.checksum }
 
   let(:repository_double) { double(:repository) }
+  let(:original_repository_double) { double(:repository) }
   let(:repository_checksum) { repository.checksum }
 
   before do
@@ -14,10 +16,16 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
     allow(Gitlab::Git::Repository).to receive(:new)
       .with('test_second_storage', project.repository.raw.relative_path, project.repository.gl_repository, project.repository.full_path)
       .and_return(project_repository_double)
+    allow(Gitlab::Git::Repository).to receive(:new)
+      .with('default', project.repository.raw.relative_path, nil, nil)
+      .and_return(original_project_repository_double)
 
     allow(Gitlab::Git::Repository).to receive(:new)
       .with('test_second_storage', repository.raw.relative_path, repository.gl_repository, repository.full_path)
       .and_return(repository_double)
+    allow(Gitlab::Git::Repository).to receive(:new)
+      .with('default', repository.raw.relative_path, nil, nil)
+      .and_return(original_repository_double)
   end
 
   context 'when the move succeeds', :clean_gitlab_redis_shared_state do
@@ -35,8 +43,8 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
       allow(repository_double).to receive(:checksum)
         .and_return(repository_checksum)
 
-      expect(GitlabShellWorker).to receive(:perform_async).with(:mv_repository, 'default', anything, anything)
-        .twice.and_call_original
+      expect(original_project_repository_double).to receive(:remove)
+      expect(original_repository_double).to receive(:remove)
     end
 
     it "moves the project and its #{repository_type} repository to the new storage and unmarks the repository as read only" do
@@ -110,13 +118,36 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
         .with(repository.raw)
         .and_raise(Gitlab::Git::CommandError)
 
-      expect(GitlabShellWorker).not_to receive(:perform_async)
-
       result = subject.execute
 
       expect(result).to be_error
       expect(project).not_to be_repository_read_only
       expect(project.repository_storage).to eq('default')
+      expect(repository_storage_move).to be_failed
+    end
+  end
+
+  context "when the cleanup of the #{repository_type} repository fails" do
+    it 'sets the correct state' do
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
+      allow(project_repository_double).to receive(:replicate)
+        .with(project.repository.raw)
+      allow(project_repository_double).to receive(:checksum)
+        .and_return(project_repository_checksum)
+      allow(original_project_repository_double).to receive(:remove)
+      allow(repository_double).to receive(:replicate)
+        .with(repository.raw)
+      allow(repository_double).to receive(:checksum)
+        .and_return(repository_checksum)
+
+      expect(original_repository_double).to receive(:remove)
+        .and_raise(Gitlab::Git::CommandError)
+
+      result = subject.execute
+
+      expect(result).to be_error
+      expect(repository_storage_move).to be_cleanup_failed
     end
   end
 
@@ -134,8 +165,6 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
       allow(repository_double).to receive(:checksum)
         .and_return('not matching checksum')
 
-      expect(GitlabShellWorker).not_to receive(:perform_async)
-
       result = subject.execute
 
       expect(result).to be_error