Merge branch 'use_replicate_repo_for_repo_move' into 'master'

Use ReplicateRepository when moving repo storage

See merge request gitlab-org/gitlab!26550

app/services/projects/update_repository_storage_service.rb

@@ -5,19 +5,12 @@ module Projects
     include Gitlab::ShellAdapter
 
     Error = Class.new(StandardError)
-    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
-
       mirror_repositories(new_repository_storage_key)
 
       mark_old_paths_for_archive
@@ -30,7 +23,7 @@ module Projects
 
       success
 
-    rescue Error => e
+    rescue Error, ArgumentError, Gitlab::Git::BaseError => e
       project.update(repository_read_only: false)
 
       Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path)
@@ -65,10 +58,7 @@ module Projects
                                                    raw_repository.gl_repository,
                                                    full_path)
 
-      unless new_repository.fetch_repository_as_mirror(raw_repository)
-        raise Error, s_('UpdateRepositoryStorage|Failed to fetch %{type} repository as mirror') % { type: type.name }
-      end
-
+      new_repository.replicate(raw_repository)
       new_checksum = new_repository.checksum
 
       if checksum != new_checksum

app/workers/project_update_repository_storage_worker.rb

@@ -9,7 +9,5 @@ class ProjectUpdateRepositoryStorageWorker # rubocop:disable Scalability/Idempot
     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}" # rubocop:disable Gitlab/RailsLogger
   end
 end

changelogs/unreleased/use_replicate_repo_for_repo_move.yml

+---
+title: Use ReplicateRepository when moving repo storage
+merge_request: 26550
+author:
+type: changed

lib/gitlab/git/repository.rb

@@ -152,6 +152,12 @@ module Gitlab
         end
       end
 
+      def replicate(source_repository)
+        wrapped_gitaly_errors do
+          gitaly_repository_client.replicate(source_repository)
+        end
+      end
+
       def expire_has_local_branches_cache
         clear_memoization(:has_local_branches)
       end
@@ -767,12 +773,6 @@ module Gitlab
         !has_visible_content?
       end
 
-      def fetch_repository_as_mirror(repository)
-        wrapped_gitaly_errors do
-          gitaly_remote_client.fetch_internal_remote(repository)
-        end
-      end
-
       # Fetch remote for repository
       #
       # remote - remote name

lib/gitlab/gitaly_client/remote_service.rb

@@ -41,20 +41,6 @@ module Gitlab
         GitalyClient.call(@storage, :remote_service, :remove_remote, request, timeout: GitalyClient.long_timeout).result
       end
 
-      def fetch_internal_remote(repository)
-        request = Gitaly::FetchInternalRemoteRequest.new(
-          repository: @gitaly_repo,
-          remote_repository: repository.gitaly_repository
-        )
-
-        response = GitalyClient.call(@storage, :remote_service,
-                                     :fetch_internal_remote, request,
-                                     timeout: GitalyClient.long_timeout,
-                                     remote_storage: repository.storage)
-
-        response.result
-      end
-
       def find_remote_root_ref(remote_name)
         request = Gitaly::FindRemoteRootRefRequest.new(
           repository: @gitaly_repo,

lib/gitlab/gitaly_client/repository_service.rb

@@ -359,6 +359,22 @@ module Gitlab
         GitalyClient.call(@storage, :repository_service, :remove_repository, request, timeout: GitalyClient.long_timeout)
       end
 
+      def replicate(source_repository)
+        request = Gitaly::ReplicateRepositoryRequest.new(
+          repository: @gitaly_repo,
+          source: source_repository.gitaly_repository
+        )
+
+        GitalyClient.call(
+          @storage,
+          :repository_service,
+          :replicate_repository,
+          request,
+          remote_storage: source_repository.storage,
+          timeout: GitalyClient.long_timeout
+        )
+      end
+
       private
 
       def search_results_from_response(gitaly_response, options = {})

lib/gitlab/setup_helper.rb

@@ -33,8 +33,6 @@ module Gitlab
 
         if Rails.env.test?
           storage_path = Rails.root.join('tmp', 'tests', 'second_storage').to_s
-
-          FileUtils.mkdir(storage_path) unless File.exist?(storage_path)
           storages << { name: 'test_second_storage', path: storage_path }
         end
 

locale/gitlab.pot

@@ -21376,9 +21376,6 @@ 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 ""
 

spec/lib/gitlab/git/repository_spec.rb

@@ -492,50 +492,6 @@ describe Gitlab::Git::Repository, :seed_helper do
     end
   end
 
-  describe '#fetch_repository_as_mirror' do
-    let(:new_repository) do
-      Gitlab::Git::Repository.new('default', 'my_project.git', '', 'group/project')
-    end
-
-    subject { new_repository.fetch_repository_as_mirror(repository) }
-
-    before do
-      new_repository.create_repository
-    end
-
-    after do
-      new_repository.remove
-    end
-
-    it 'fetches a repository as a mirror remote' do
-      subject
-
-      expect(refs(new_repository_path)).to eq(refs(repository_path))
-    end
-
-    context 'with keep-around refs' do
-      let(:sha) { SeedRepo::Commit::ID }
-      let(:keep_around_ref) { "refs/keep-around/#{sha}" }
-      let(:tmp_ref) { "refs/tmp/#{SecureRandom.hex}" }
-
-      before do
-        repository_rugged.references.create(keep_around_ref, sha, force: true)
-        repository_rugged.references.create(tmp_ref, sha, force: true)
-      end
-
-      it 'includes the temporary and keep-around refs' do
-        subject
-
-        expect(refs(new_repository_path)).to include(keep_around_ref)
-        expect(refs(new_repository_path)).to include(tmp_ref)
-      end
-    end
-
-    def new_repository_path
-      File.join(TestEnv.repos_path, new_repository.relative_path)
-    end
-  end
-
   describe '#fetch_remote' do
     it 'delegates to the gitaly RepositoryService' do
       ssh_auth = double(:ssh_auth)
@@ -2181,4 +2137,49 @@ describe Gitlab::Git::Repository, :seed_helper do
       end
     end
   end
+
+  describe '#replicate' do
+    let(:new_repository) do
+      Gitlab::Git::Repository.new('test_second_storage', TEST_REPO_PATH, '', 'group/project')
+    end
+    let(:new_repository_path) { File.join(TestEnv::SECOND_STORAGE_PATH, new_repository.relative_path) }
+
+    subject { new_repository.replicate(repository) }
+
+    before do
+      stub_storage_settings('test_second_storage' => {
+        'gitaly_address' => Gitlab.config.repositories.storages.default.gitaly_address,
+        'path' => TestEnv::SECOND_STORAGE_PATH
+      })
+      Gitlab::Shell.new.create_repository('test_second_storage', TEST_REPO_PATH, 'group/project')
+    end
+
+    after do
+      Gitlab::Shell.new.remove_repository('test_second_storage', TEST_REPO_PATH)
+    end
+
+    it 'mirrors the source repository' do
+      subject
+
+      expect(refs(new_repository_path)).to eq(refs(repository_path))
+    end
+
+    context 'with keep-around refs' do
+      let(:sha) { SeedRepo::Commit::ID }
+      let(:keep_around_ref) { "refs/keep-around/#{sha}" }
+      let(:tmp_ref) { "refs/tmp/#{SecureRandom.hex}" }
+
+      before do
+        repository.write_ref(keep_around_ref, sha)
+        repository.write_ref(tmp_ref, sha)
+      end
+
+      it 'includes the temporary and keep-around refs' do
+        subject
+
+        expect(refs(new_repository_path)).to include(keep_around_ref)
+        expect(refs(new_repository_path)).to include(tmp_ref)
+      end
+    end
+  end
 end

spec/lib/gitlab/gitaly_client/remote_service_spec.rb

@@ -34,19 +34,6 @@ describe Gitlab::GitalyClient::RemoteService do
     end
   end
 
-  describe '#fetch_internal_remote' do
-    let(:remote_repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '', 'group/project') }
-
-    it 'sends an fetch_internal_remote message and returns the result value' do
-      expect_any_instance_of(Gitaly::RemoteService::Stub)
-        .to receive(:fetch_internal_remote)
-        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
-        .and_return(double(result: true))
-
-      expect(client.fetch_internal_remote(remote_repository)).to be(true)
-    end
-  end
-
   describe '#find_remote_root_ref' do
     it 'sends an find_remote_root_ref message and returns the root ref' do
       expect_any_instance_of(Gitaly::RemoteService::Stub)

spec/lib/gitlab/gitaly_client/repository_service_spec.rb

@@ -275,7 +275,18 @@ describe Gitlab::GitalyClient::RepositoryService do
     end
   end
 
-  describe 'remove' do
+  describe '#rename' do
+    it 'sends a rename_repository message' do
+      expect_any_instance_of(Gitaly::RepositoryService::Stub)
+        .to receive(:rename_repository)
+        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+        .and_return(double(value: true))
+
+      client.rename('some/new/path')
+    end
+  end
+
+  describe '#remove' do
     it 'sends a remove_repository message' do
       expect_any_instance_of(Gitaly::RepositoryService::Stub)
         .to receive(:remove_repository)
@@ -286,14 +297,15 @@ describe Gitlab::GitalyClient::RepositoryService do
     end
   end
 
-  describe 'rename' do
-    it 'sends a rename_repository message' do
+  describe '#replicate' do
+    let(:source_repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '', 'group/project') }
+
+    it 'sends a replicate_repository message' do
       expect_any_instance_of(Gitaly::RepositoryService::Stub)
-        .to receive(:rename_repository)
+        .to receive(:replicate_repository)
         .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
-        .and_return(double(value: true))
 
-      client.rename('some/new/path')
+      client.replicate(source_repository)
     end
   end
 end

spec/requests/api/projects_spec.rb

@@ -2455,7 +2455,7 @@ describe API::Projects do
         end
 
         it 'returns 200 when repository storage has changed' do
-          stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/second_storage' })
+          stub_storage_settings('test_second_storage' => { 'path' => TestEnv::SECOND_STORAGE_PATH })
 
           expect do
             Sidekiq::Testing.fake! do

spec/services/projects/fork_service_spec.rb

@@ -311,9 +311,10 @@ describe Projects::ForkService do
       fork_before_move = fork_project(project)
 
       # Stub everything required to move a project to a Gitaly shard that does not exist
-      stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/second_storage' })
-      allow_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror).and_return(true)
-      allow_any_instance_of(Gitlab::Git::Repository).to receive(:checksum).and_return(::Gitlab::Git::BLANK_SHA)
+      stub_storage_settings('test_second_storage' => { 'path' => TestEnv::SECOND_STORAGE_PATH })
+      allow_any_instance_of(Gitlab::Git::Repository).to receive(:replicate)
+      allow_any_instance_of(Gitlab::Git::Repository).to receive(:checksum)
+        .and_return(::Gitlab::Git::BLANK_SHA)
 
       Projects::UpdateRepositoryStorageService.new(project).execute('test_second_storage')
       fork_after_move = fork_project(project)

spec/services/projects/update_repository_storage_service_spec.rb

@@ -32,8 +32,8 @@ describe Projects::UpdateRepositoryStorageService do
             project.repository.path_to_repo
           end
 
-          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
-            .with(project.repository.raw).and_return(true)
+          expect(project_repository_double).to receive(:replicate)
+            .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
             .and_return(checksum)
 
@@ -49,16 +49,18 @@ describe Projects::UpdateRepositoryStorageService do
 
       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)
+          result = subject.execute(project.repository_storage)
+
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to match(/repository and source have the same storage/)
         end
       end
 
       context 'when the move fails' do
         it 'unmarks the repository as read-only without updating the repository storage' do
-          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
-            .with(project.repository.raw).and_return(false)
+          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('test_second_storage')
@@ -71,8 +73,8 @@ describe Projects::UpdateRepositoryStorageService do
 
       context 'when the checksum does not match' do
         it 'unmarks the repository as read-only without updating the repository storage' do
-          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
-            .with(project.repository.raw).and_return(true)
+          expect(project_repository_double).to receive(:replicate)
+            .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
             .and_return('not matching checksum')
           expect(GitlabShellWorker).not_to receive(:perform_async)
@@ -89,8 +91,8 @@ describe Projects::UpdateRepositoryStorageService do
         let!(:pool) { create(:pool_repository, :ready, source_project: project) }
 
         it 'leaves the pool' do
-          expect(project_repository_double).to receive(:fetch_repository_as_mirror)
-            .with(project.repository.raw).and_return(true)
+          expect(project_repository_double).to receive(:replicate)
+            .with(project.repository.raw)
           expect(project_repository_double).to receive(:checksum)
             .and_return(checksum)
 

spec/support/helpers/test_env.rb

@@ -84,6 +84,7 @@ module TestEnv
 
   TMP_TEST_PATH = Rails.root.join('tmp', 'tests', '**')
   REPOS_STORAGE = 'default'.freeze
+  SECOND_STORAGE_PATH = Rails.root.join('tmp', 'tests', 'second_storage')
 
   # Test environment
   #
@@ -141,6 +142,7 @@ module TestEnv
     end
 
     FileUtils.mkdir_p(repos_path)
+    FileUtils.mkdir_p(SECOND_STORAGE_PATH)
     FileUtils.mkdir_p(backup_path)
     FileUtils.mkdir_p(pages_path)
     FileUtils.mkdir_p(artifacts_path)
@@ -176,8 +178,6 @@ module TestEnv
       return
     end
 
-    FileUtils.mkdir_p("tmp/tests/second_storage") unless File.exist?("tmp/tests/second_storage")
-
     spawn_script = Rails.root.join('scripts/gitaly-test-spawn').to_s
     Bundler.with_original_env do
       unless system(spawn_script)

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

@@ -22,14 +22,13 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context 'when the move succeeds', :clean_gitlab_redis_shared_state do
     before do
-      allow(project_repository_double).to receive(:fetch_repository_as_mirror)
+      allow(project_repository_double).to receive(:replicate)
         .with(project.repository.raw)
-        .and_return(true)
       allow(project_repository_double).to receive(:checksum)
         .and_return(project_repository_checksum)
 
-      allow(repository_double).to receive(:fetch_repository_as_mirror)
-        .with(repository.raw).and_return(true)
+      allow(repository_double).to receive(:replicate)
+        .with(repository.raw)
       allow(repository_double).to receive(:checksum)
         .and_return(repository_checksum)
     end
@@ -82,20 +81,23 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   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)
+      result = subject.execute(project.repository_storage)
+
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to match(/repository and source have the same storage/)
     end
   end
 
   context "when the move of the #{repository_type} repository fails" do
     it 'unmarks the repository as read-only without updating the repository storage' do
-      allow(project_repository_double).to receive(:fetch_repository_as_mirror)
-        .with(project.repository.raw).and_return(true)
+      allow(project_repository_double).to receive(:replicate)
+        .with(project.repository.raw)
       allow(project_repository_double).to receive(:checksum)
         .and_return(project_repository_checksum)
-      allow(repository_double).to receive(:fetch_repository_as_mirror)
-        .with(repository.raw).and_return(false)
+
+      allow(repository_double).to receive(:replicate)
+        .with(repository.raw)
+        .and_raise(Gitlab::Git::CommandError)
 
       expect(GitlabShellWorker).not_to receive(:perform_async)
 
@@ -109,13 +111,13 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context "when the checksum of the #{repository_type} repository does not match" do
     it 'unmarks the repository as read-only without updating the repository storage' do
-      allow(project_repository_double).to receive(:fetch_repository_as_mirror)
-        .with(project.repository.raw).and_return(true)
+      allow(project_repository_double).to receive(:replicate)
+        .with(project.repository.raw)
       allow(project_repository_double).to receive(:checksum)
         .and_return(project_repository_checksum)
 
-      allow(repository_double).to receive(:fetch_repository_as_mirror)
-        .with(repository.raw).and_return(true)
+      allow(repository_double).to receive(:replicate)
+        .with(repository.raw)
       allow(repository_double).to receive(:checksum)
         .and_return('not matching checksum')
 

spec/workers/project_update_repository_storage_worker_spec.rb

@@ -15,11 +15,5 @@ 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