Merge branch 'gitaly-fetch-repository' into 'master'

Add feature flag to use gitaly-ssh mirroring when cloning internal repos

Closes gitaly#739

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

ee/app/services/projects/update_repository_storage_service.rb

@@ -35,7 +35,7 @@ module Projects
                                                    repository.relative_path,
                                                    repository.gl_repository)
 
-      new_repository.fetch_as_mirror_without_shell(repository.path)
+      new_repository.fetch_repository_as_mirror(repository)
     end
 
     def mark_old_paths_for_archive

lib/gitlab/git/repository.rb

@@ -18,6 +18,7 @@ module Gitlab
         GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE
       ].freeze
       SEARCH_CONTEXT_LINES = 3
+      GITALY_INTERNAL_URL = 'ssh://gitaly/internal.git'.freeze
 
       NoRepository = Class.new(StandardError)
       InvalidBlobName = Class.new(StandardError)
@@ -1150,12 +1151,25 @@ module Gitlab
         @has_visible_content = has_local_branches?
       end
 
-      # Like all public `Gitlab::Git::Repository` methods, this method is part
-      # of `Repository`'s interface through `method_missing`.
-      # `Repository` has its own `fetch_remote` which uses `gitlab-shell` and
-      # takes some extra attributes, so we qualify this method name to prevent confusion.
-      def fetch_remote_without_shell(remote = 'origin')
-        run_git(['fetch', remote]).last.zero?
+      def fetch_repository_as_mirror(repository)
+        remote_name = "tmp-#{SecureRandom.hex}"
+
+        # Notice that this feature flag is not for `fetch_repository_as_mirror`
+        # as a whole but for the fetching mechanism (file path or gitaly-ssh).
+        url, env = gitaly_migrate(:fetch_internal) do |is_enabled|
+          if is_enabled
+            repository = RemoteRepository.new(repository) unless repository.is_a?(RemoteRepository)
+            [GITALY_INTERNAL_URL, repository.fetch_env]
+          else
+            [repository.path, nil]
+          end
+        end
+
+        add_remote(remote_name, url)
+        set_remote_as_mirror(remote_name)
+        fetch_remote(remote_name, env: env)
+      ensure
+        remove_remote(remote_name)
       end
 
       def blob_at(sha, path)
@@ -1661,7 +1675,7 @@ module Gitlab
       end
 
       def gitaly_fetch_ref(source_repository, source_ref:, target_ref:)
-        args = %W(fetch --no-tags -f ssh://gitaly/internal.git #{source_ref}:#{target_ref})
+        args = %W(fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref})
 
         run_git(args, env: source_repository.fetch_env)
       end
@@ -1681,6 +1695,10 @@ module Gitlab
       rescue Rugged::ReferenceError
         raise ArgumentError, 'Invalid merge source'
       end
+
+      def fetch_remote(remote_name = 'origin', env: nil)
+        run_git(['fetch', remote_name], env: env).last.zero?
+      end
     end
   end
 end

lib/gitlab/git/repository_mirroring.rb

@@ -31,19 +31,6 @@ module Gitlab
         end
       end
 
-      # Like all_refs public `Gitlab::Git::Repository` methods, this method is part
-      # of `Repository`'s interface through `method_missing`.
-      # `Repository` has its own `fetch_as_mirror` which uses `gitlab-shell` and
-      # takes some extra attributes, so we qualify this method name to prevent confusion.
-      def fetch_as_mirror_without_shell(url)
-        remote_name = "tmp-#{SecureRandom.hex}"
-        add_remote(remote_name, url)
-        set_remote_as_mirror(remote_name)
-        fetch_remote_without_shell(remote_name)
-      ensure
-        remove_remote(remote_name) if remote_name
-      end
-
       def remote_tags(remote)
         # Each line has this format: "dc872e9fa6963f8f03da6c8f6f264d0845d6b092\trefs/tags/v1.10.0\n"
         # We want to convert it to: [{ 'v1.10.0' => 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' }, ...]

spec/lib/gitlab/git/repository_spec.rb

@@ -588,12 +588,12 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  describe '#fetch_as_mirror_without_shell' do
+  describe '#fetch_repository_as_mirror' do
     let(:new_repository) do
       Gitlab::Git::Repository.new('default', 'my_project.git', '')
     end
 
-    subject { new_repository.fetch_as_mirror_without_shell(repository.path) }
+    subject { new_repository.fetch_repository_as_mirror(repository) }
 
     before do
       Gitlab::Shell.new.add_repository('default', 'my_project')
@@ -603,7 +603,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
       Gitlab::Shell.new.remove_repository(TestEnv.repos_path, 'my_project')
     end
 
-    it 'fetches a url as a mirror remote' do
+    it 'fetches a repository as a mirror remote' do
       subject
 
       expect(refs(new_repository.path)).to eq(refs(repository.path))
@@ -1652,21 +1652,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  describe '#fetch_remote_without_shell' do
-    let(:git_path) { Gitlab.config.git.bin_path }
-    let(:remote_name) { 'my_remote' }
-
-    subject { repository.fetch_remote_without_shell(remote_name) }
-
-    it 'fetches the remote and returns true if the command was successful' do
-      expect(repository).to receive(:popen)
-        .with(%W(#{git_path} fetch #{remote_name}), repository.path, {})
-        .and_return(['', 0])
-
-      expect(subject).to be(true)
-    end
-  end
-
   describe '#merge' do
     let(:repository) do
       Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')

spec/services/projects/update_repository_storage_service_spec.rb

@@ -31,8 +31,8 @@ describe Projects::UpdateRepositoryStorageService do
 
       context 'when the move succeeds' do
         it 'moves the repository to the new storage and unmarks the repository as read only' do
-          expect_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_as_mirror_without_shell)
-            .with(project.repository.raw.path).and_return(true)
+          expect_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror)
+            .with(project.repository.raw).and_return(true)
           expect(GitlabShellWorker).to receive(:perform_async)
             .with(:mv_repository,
               'tmp/tests/storage_a',
@@ -48,8 +48,8 @@ describe Projects::UpdateRepositoryStorageService do
 
       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_as_mirror_without_shell)
-            .with(project.repository.raw.path).and_return(false)
+          expect_any_instance_of(Gitlab::Git::Repository).to receive(:fetch_repository_as_mirror)
+            .with(project.repository.raw).and_return(false)
           expect(GitlabShellWorker).not_to receive(:perform_async)
 
           subject.execute('b')
@@ -84,16 +84,16 @@ describe Projects::UpdateRepositoryStorageService do
 
       context 'when the move succeeds' do
         it 'moves the repository and its wiki to the new storage and unmarks the repository as read only' do
-          expect(repository_double).to receive(:fetch_as_mirror_without_shell)
-            .with(project.repository.raw.path).and_return(true)
+          expect(repository_double).to receive(:fetch_repository_as_mirror)
+            .with(project.repository.raw).and_return(true)
           expect(GitlabShellWorker).to receive(:perform_async)
             .with(:mv_repository,
               'tmp/tests/storage_a',
               project.disk_path,
               "#{project.disk_path}+#{project.id}+moved+#{time.to_i}")
 
-          expect(wiki_repository_double).to receive(:fetch_as_mirror_without_shell)
-            .with(project.wiki.repository.raw.path).and_return(true)
+          expect(wiki_repository_double).to receive(:fetch_repository_as_mirror)
+            .with(project.wiki.repository.raw).and_return(true)
           expect(GitlabShellWorker).to receive(:perform_async)
             .with(:mv_repository,
               'tmp/tests/storage_a',
@@ -109,10 +109,10 @@ describe Projects::UpdateRepositoryStorageService do
 
       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_as_mirror_without_shell)
-            .with(project.repository.raw.path).and_return(true)
-          expect(wiki_repository_double).to receive(:fetch_as_mirror_without_shell)
-            .with(project.wiki.repository.raw.path).and_return(false)
+          expect(repository_double).to receive(:fetch_repository_as_mirror)
+            .with(project.repository.raw).and_return(true)
+          expect(wiki_repository_double).to receive(:fetch_repository_as_mirror)
+            .with(project.wiki.repository.raw).and_return(false)
           expect(GitlabShellWorker).not_to receive(:perform_async)
 
           subject.execute('b')