Merge branch 'fix/use-shard-name-in-gitlab-projects-instead-of-shard-path' into 'master'

Use shard name in Git::GitlabProjects instead of shard path

Closes gitaly#1110

See merge request gitlab-org/gitlab-ce!18015

app/models/project.rb

@@ -566,9 +566,7 @@ class Project < ActiveRecord::Base
   def add_import_job
     job_id =
       if forked?
-        RepositoryForkWorker.perform_async(id,
-                                           forked_from_project.repository_storage_path,
-                                           forked_from_project.disk_path)
+        RepositoryForkWorker.perform_async(id)
       elsif gitlab_project_import?
         # Do not retry on Import/Export until https://gitlab.com/gitlab-org/gitlab-ce/issues/26189 is solved.
         RepositoryImportWorker.set(retry: false).perform_async(self.id)

app/services/projects/import_service.rb

@@ -61,7 +61,7 @@ module Projects
           project.ensure_repository
           project.repository.fetch_as_mirror(project.import_url, refmap: refmap)
         else
-          gitlab_shell.import_repository(project.repository_storage_path, project.disk_path, project.import_url)
+          gitlab_shell.import_repository(project.repository_storage, project.disk_path, project.import_url)
         end
       rescue Gitlab::Shell::Error, Gitlab::Git::RepositoryMirroring::RemoteError => e
         # Expire cache to prevent scenarios such as:

app/workers/repository_fork_worker.rb

-# Gitaly issue: https://gitlab.com/gitlab-org/gitaly/issues/1110
 class RepositoryForkWorker
   include ApplicationWorker
   include Gitlab::ShellAdapter
   include ProjectStartImport
   include ProjectImportOptions
 
-  def perform(project_id, forked_from_repository_storage_path, source_disk_path)
-    project = Project.find(project_id)
+  def perform(*args)
+    target_project_id = args.shift
+    target_project = Project.find(target_project_id)
 
-    return unless start_fork(project)
+    # By v10.8, we should've drained the queue of all jobs using the old arguments.
+    # We can remove the else clause if we're no longer logging the message in that clause.
+    # See https://gitlab.com/gitlab-org/gitaly/issues/1110
+    if args.empty?
+      source_project = target_project.forked_from_project
+      return target_project.mark_import_as_failed('Source project cannot be found.') unless source_project
 
-    Gitlab::Metrics.add_event(:fork_repository,
-                              source_path: source_disk_path,
-                              target_path: project.disk_path)
+      fork_repository(target_project, source_project.repository_storage, source_project.disk_path)
+    else
+      Rails.logger.info("Project #{target_project.id} is being forked using old-style arguments.")
+
+      source_repository_storage_path, source_disk_path = *args
 
-    result = gitlab_shell.fork_repository(forked_from_repository_storage_path, source_disk_path,
-                                          project.repository_storage_path, project.disk_path)
-    raise "Unable to fork project #{project_id} for repository #{source_disk_path} -> #{project.disk_path}" unless result
+      source_repository_storage_name = Gitlab.config.repositories.storages.find do |_, info|
+        info.legacy_disk_path == source_repository_storage_path
+      end&.first || raise("no shard found for path '#{source_repository_storage_path}'")
 
-    project.after_import
+      fork_repository(target_project, source_repository_storage_name, source_disk_path)
+    end
   end
 
   private
 
+  def fork_repository(target_project, source_repository_storage_name, source_disk_path)
+    return unless start_fork(target_project)
+
+    Gitlab::Metrics.add_event(:fork_repository,
+                              source_path: source_disk_path,
+                              target_path: target_project.disk_path)
+
+    result = gitlab_shell.fork_repository(source_repository_storage_name, source_disk_path,
+                                          target_project.repository_storage, target_project.disk_path)
+    raise "Unable to fork project #{target_project.id} for repository #{source_disk_path} -> #{target_project.disk_path}" unless result
+
+    target_project.after_import
+  end
+
   def start_fork(project)
     return true if start(project)
 

lib/gitlab/bitbucket_import/importer.rb

@@ -63,7 +63,7 @@ module Gitlab
 
         disk_path = project.wiki.disk_path
         import_url = project.import_url.sub(/\.git\z/, ".git/wiki")
-        gitlab_shell.import_repository(project.repository_storage_path, disk_path, import_url)
+        gitlab_shell.import_repository(project.repository_storage, disk_path, import_url)
       rescue StandardError => e
         errors << { type: :wiki, errors: e.message }
       end

lib/gitlab/git/gitlab_projects.rb

@@ -4,20 +4,14 @@ module Gitlab
       include Gitlab::Git::Popen
       include Gitlab::Utils::StrongMemoize
 
-      ShardNameNotFoundError = Class.new(StandardError)
-
-      # Absolute path to directory where repositories are stored.
-      # Example: /home/git/repositories
-      attr_reader :shard_path
+      # Name of shard where repositories are stored.
+      # Example: nfs-file06
+      attr_reader :shard_name
 
       # Relative path is a directory name for repository with .git at the end.
       # Example: gitlab-org/gitlab-test.git
       attr_reader :repository_relative_path
 
-      # Absolute path to the repository.
-      # Example: /home/git/repositorities/gitlab-org/gitlab-test.git
-      attr_reader :repository_absolute_path
-
       # This is the path at which the gitlab-shell hooks directory can be found.
       # It's essential for integration between git and GitLab proper. All new
       # repositories should have their hooks directory symlinked here.
@@ -25,13 +19,12 @@ module Gitlab
 
       attr_reader :logger
 
-      def initialize(shard_path, repository_relative_path, global_hooks_path:, logger:)
-        @shard_path = shard_path
+      def initialize(shard_name, repository_relative_path, global_hooks_path:, logger:)
+        @shard_name = shard_name
         @repository_relative_path = repository_relative_path
 
         @logger = logger
         @global_hooks_path = global_hooks_path
-        @repository_absolute_path = File.join(shard_path, repository_relative_path)
         @output = StringIO.new
       end
 
@@ -41,6 +34,22 @@ module Gitlab
         io.read
       end
 
+      # Absolute path to the repository.
+      # Example: /home/git/repositorities/gitlab-org/gitlab-test.git
+      # Probably will be removed when we fully migrate to Gitaly, part of
+      # https://gitlab.com/gitlab-org/gitaly/issues/1124.
+      def repository_absolute_path
+        strong_memoize(:repository_absolute_path) do
+          File.join(shard_path, repository_relative_path)
+        end
+      end
+
+      def shard_path
+        strong_memoize(:shard_path) do
+          Gitlab.config.repositories.storages.fetch(shard_name).legacy_disk_path
+        end
+      end
+
       # Import project via git clone --bare
       # URL must be publicly cloneable
       def import_project(source, timeout)
@@ -53,12 +62,12 @@ module Gitlab
         end
       end
 
-      def fork_repository(new_shard_path, new_repository_relative_path)
+      def fork_repository(new_shard_name, new_repository_relative_path)
         Gitlab::GitalyClient.migrate(:fork_repository) do |is_enabled|
           if is_enabled
-            gitaly_fork_repository(new_shard_path, new_repository_relative_path)
+            gitaly_fork_repository(new_shard_name, new_repository_relative_path)
           else
-            git_fork_repository(new_shard_path, new_repository_relative_path)
+            git_fork_repository(new_shard_name, new_repository_relative_path)
           end
         end
       end
@@ -205,17 +214,6 @@ module Gitlab
 
       private
 
-      def shard_name
-        strong_memoize(:shard_name) do
-          shard_name_from_shard_path(shard_path)
-        end
-      end
-
-      def shard_name_from_shard_path(shard_path)
-        Gitlab.config.repositories.storages.find { |_, info| info.legacy_disk_path == shard_path }&.first ||
-          raise(ShardNameNotFoundError, "no shard found for path '#{shard_path}'")
-      end
-
       def git_import_repository(source, timeout)
         # Skip import if repo already exists
         return false if File.exist?(repository_absolute_path)
@@ -252,8 +250,9 @@ module Gitlab
         false
       end
 
-      def git_fork_repository(new_shard_path, new_repository_relative_path)
+      def git_fork_repository(new_shard_name, new_repository_relative_path)
         from_path = repository_absolute_path
+        new_shard_path = Gitlab.config.repositories.storages.fetch(new_shard_name).legacy_disk_path
         to_path = File.join(new_shard_path, new_repository_relative_path)
 
         # The repository cannot already exist
@@ -271,8 +270,8 @@ module Gitlab
         run(cmd, nil) && Gitlab::Git::Repository.create_hooks(to_path, global_hooks_path)
       end
 
-      def gitaly_fork_repository(new_shard_path, new_repository_relative_path)
-        target_repository = Gitlab::Git::Repository.new(shard_name_from_shard_path(new_shard_path), new_repository_relative_path, nil)
+      def gitaly_fork_repository(new_shard_name, new_repository_relative_path)
+        target_repository = Gitlab::Git::Repository.new(new_shard_name, new_repository_relative_path, nil)
         raw_repository = Gitlab::Git::Repository.new(shard_name, repository_relative_path, nil)
 
         Gitlab::GitalyClient::RepositoryService.new(target_repository).fork_repository(raw_repository)

lib/gitlab/git/repository.rb

@@ -96,7 +96,7 @@ module Gitlab
 
         storage_path = Gitlab.config.repositories.storages[@storage].legacy_disk_path
         @gitlab_projects = Gitlab::Git::GitlabProjects.new(
-          storage_path,
+          storage,
           relative_path,
           global_hooks_path: Gitlab.config.gitlab_shell.hooks_path,
           logger: Rails.logger

lib/gitlab/github_import/importer/repository_importer.rb

@@ -56,9 +56,8 @@ module Gitlab
 
         def import_wiki_repository
           wiki_path = "#{project.disk_path}.wiki"
-          storage_path = project.repository_storage_path
 
-          gitlab_shell.import_repository(storage_path, wiki_path, wiki_url)
+          gitlab_shell.import_repository(project.repository_storage, wiki_path, wiki_url)
 
           true
         rescue Gitlab::Shell::Error => e

lib/gitlab/legacy_github_import/importer.rb

@@ -259,7 +259,7 @@ module Gitlab
       def import_wiki
         unless project.wiki.repository_exists?
           wiki = WikiFormatter.new(project)
-          gitlab_shell.import_repository(project.repository_storage_path, wiki.disk_path, wiki.import_url)
+          gitlab_shell.import_repository(project.repository_storage, wiki.disk_path, wiki.import_url)
         end
       rescue Gitlab::Shell::Error => e
         # GitHub error message when the wiki repo has not been created,

lib/gitlab/shell.rb

@@ -93,12 +93,12 @@ module Gitlab
 
     # Import repository
     #
-    # storage - project's storage path
+    # storage - project's storage name
     # name - project disk path
     # url - URL to import from
     #
     # Ex.
-    #   import_repository("/path/to/storage", "gitlab/gitlab-ci", "https://gitlab.com/gitlab-org/gitlab-test.git")
+    #   import_repository("nfs-file06", "gitlab/gitlab-ci", "https://gitlab.com/gitlab-org/gitlab-test.git")
     #
     # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/874
     def import_repository(storage, name, url)
@@ -131,8 +131,7 @@ module Gitlab
         if is_enabled
           repository.gitaly_repository_client.fetch_remote(remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, timeout: git_timeout, prune: prune)
         else
-          storage_path = Gitlab.config.repositories.storages[repository.storage].legacy_disk_path
-          local_fetch_remote(storage_path, repository.relative_path, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, prune: prune)
+          local_fetch_remote(repository.storage, repository.relative_path, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, prune: prune)
         end
       end
     end
@@ -156,13 +155,13 @@ module Gitlab
     end
 
     # Fork repository to new path
-    # forked_from_storage - forked-from project's storage path
-    # forked_from_disk_path - project disk path
-    # forked_to_storage - forked-to project's storage path
-    # forked_to_disk_path - forked project disk path
+    # forked_from_storage - forked-from project's storage name
+    # forked_from_disk_path - project disk relative path
+    # forked_to_storage - forked-to project's storage name
+    # forked_to_disk_path - forked project disk relative path
     #
     # Ex.
-    #  fork_repository("/path/to/forked_from/storage", "gitlab/gitlab-ci", "/path/to/forked_to/storage", "new-namespace/gitlab-ci")
+    #  fork_repository("nfs-file06", "gitlab/gitlab-ci", "nfs-file07", "new-namespace/gitlab-ci")
     #
     # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/817
     def fork_repository(forked_from_storage, forked_from_disk_path, forked_to_storage, forked_to_disk_path)
@@ -420,16 +419,16 @@ module Gitlab
 
     private
 
-    def gitlab_projects(shard_path, disk_path)
+    def gitlab_projects(shard_name, disk_path)
       Gitlab::Git::GitlabProjects.new(
-        shard_path,
+        shard_name,
         disk_path,
         global_hooks_path: Gitlab.config.gitlab_shell.hooks_path,
         logger: Rails.logger
       )
     end
 
-    def local_fetch_remote(storage_path, repository_relative_path, remote, ssh_auth: nil, forced: false, no_tags: false, prune: true)
+    def local_fetch_remote(storage_name, repository_relative_path, remote, ssh_auth: nil, forced: false, no_tags: false, prune: true)
       vars = { force: forced, tags: !no_tags, prune: prune }
 
       if ssh_auth&.ssh_import?
@@ -442,7 +441,7 @@ module Gitlab
         end
       end
 
-      cmd = gitlab_projects(storage_path, repository_relative_path)
+      cmd = gitlab_projects(storage_name, repository_relative_path)
 
       success = cmd.fetch_remote(remote, git_timeout, vars)
 

spec/lib/gitlab/bitbucket_import/importer_spec.rb

@@ -137,7 +137,7 @@ describe Gitlab::BitbucketImport::Importer do
       it 'imports to the project disk_path' do
         expect(project.wiki).to receive(:repository_exists?) { false }
         expect(importer.gitlab_shell).to receive(:import_repository).with(
-          project.repository_storage_path,
+          project.repository_storage,
           project.wiki.disk_path,
           project.import_url + '/wiki'
         )

spec/lib/gitlab/git/gitlab_projects_spec.rb

@@ -16,7 +16,7 @@ describe Gitlab::Git::GitlabProjects do
   let(:tmp_repos_path) { TestEnv.repos_path }
   let(:repo_name) { project.disk_path + '.git' }
   let(:tmp_repo_path) { File.join(tmp_repos_path, repo_name) }
-  let(:gl_projects) { build_gitlab_projects(tmp_repos_path, repo_name) }
+  let(:gl_projects) { build_gitlab_projects(TestEnv::REPOS_STORAGE, repo_name) }
 
   describe '#initialize' do
     it { expect(gl_projects.shard_path).to eq(tmp_repos_path) }
@@ -223,11 +223,12 @@ describe Gitlab::Git::GitlabProjects do
   end
 
   describe '#fork_repository' do
+    let(:dest_repos) { TestEnv::REPOS_STORAGE }
     let(:dest_repos_path) { tmp_repos_path }
     let(:dest_repo_name) { File.join('@hashed', 'aa', 'bb', 'xyz.git') }
     let(:dest_repo) { File.join(dest_repos_path, dest_repo_name) }
 
-    subject { gl_projects.fork_repository(dest_repos_path, dest_repo_name) }
+    subject { gl_projects.fork_repository(dest_repos, dest_repo_name) }
 
     before do
       FileUtils.mkdir_p(dest_repos_path)
@@ -268,7 +269,12 @@ describe Gitlab::Git::GitlabProjects do
       # that is not very straight-forward so I'm leaving this test here for now till
       # https://gitlab.com/gitlab-org/gitlab-ce/issues/41393 is fixed.
       context 'different storages' do
-        let(:dest_repos_path) { File.join(File.dirname(tmp_repos_path), 'alternative') }
+        let(:dest_repos) { 'alternative' }
+        let(:dest_repos_path) { File.join(File.dirname(tmp_repos_path), dest_repos) }
+
+        before do
+          stub_storage_settings(dest_repos => { 'path' => dest_repos_path })
+        end
 
         it 'forks the repo' do
           is_expected.to be_truthy

spec/lib/gitlab/github_import/importer/repository_importer_spec.rb

@@ -9,7 +9,7 @@ describe Gitlab::GithubImport::Importer::RepositoryImporter do
       :project,
       import_url: 'foo.git',
       import_source: 'foo/bar',
-      repository_storage_path: 'foo',
+      repository_storage: 'foo',
       disk_path: 'foo',
       repository: repository,
       create_wiki: true

spec/lib/gitlab/shell_spec.rb

@@ -14,7 +14,7 @@ describe Gitlab::Shell do
     allow(Project).to receive(:find).and_return(project)
 
     allow(gitlab_shell).to receive(:gitlab_projects)
-      .with(project.repository_storage_path, project.disk_path + '.git')
+      .with(project.repository_storage, project.disk_path + '.git')
       .and_return(gitlab_projects)
   end
 
@@ -487,21 +487,21 @@ describe Gitlab::Shell do
     describe '#fork_repository' do
       subject do
         gitlab_shell.fork_repository(
-          project.repository_storage_path,
+          project.repository_storage,
           project.disk_path,
-          'new/storage',
+          'nfs-file05',
           'fork/path'
         )
       end
 
       it 'returns true when the command succeeds' do
-        expect(gitlab_projects).to receive(:fork_repository).with('new/storage', 'fork/path.git') { true }
+        expect(gitlab_projects).to receive(:fork_repository).with('nfs-file05', 'fork/path.git') { true }
 
         is_expected.to be_truthy
       end
 
       it 'return false when the command fails' do
-        expect(gitlab_projects).to receive(:fork_repository).with('new/storage', 'fork/path.git') { false }
+        expect(gitlab_projects).to receive(:fork_repository).with('nfs-file05', 'fork/path.git') { false }
 
         is_expected.to be_falsy
       end
@@ -661,7 +661,7 @@ describe Gitlab::Shell do
       it 'returns true when the command succeeds' do
         expect(gitlab_projects).to receive(:import_project).with(import_url, timeout) { true }
 
-        result = gitlab_shell.import_repository(project.repository_storage_path, project.disk_path, import_url)
+        result = gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
 
         expect(result).to be_truthy
       end
@@ -671,7 +671,7 @@ describe Gitlab::Shell do
         expect(gitlab_projects).to receive(:import_project) { false }
 
         expect do
-          gitlab_shell.import_repository(project.repository_storage_path, project.disk_path, import_url)
+          gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url)
         end.to raise_error(Gitlab::Shell::Error, "error")
       end
     end

spec/models/project_spec.rb

@@ -1645,7 +1645,7 @@ describe Project do
 
     before do
       allow_any_instance_of(Gitlab::Shell).to receive(:import_repository)
-        .with(project.repository_storage_path, project.disk_path, project.import_url)
+        .with(project.repository_storage, project.disk_path, project.import_url)
         .and_return(true)
 
       expect_any_instance_of(Repository).to receive(:after_import)
@@ -1798,10 +1798,7 @@ describe Project do
       let(:project) { forked_project_link.forked_to_project }
 
       it 'schedules a RepositoryForkWorker job' do
-        expect(RepositoryForkWorker).to receive(:perform_async).with(
-          project.id,
-          forked_from_project.repository_storage_path,
-          forked_from_project.disk_path).and_return(import_jid)
+        expect(RepositoryForkWorker).to receive(:perform_async).with(project.id).and_return(import_jid)
 
         expect(project.add_import_job).to eq(import_jid)
       end

spec/spec_helper.rb

@@ -108,7 +108,8 @@ RSpec.configure do |config|
     allow_any_instance_of(Gitlab::Git::GitlabProjects).to receive(:fork_repository).and_wrap_original do |m, *args|
       m.call(*args)
 
-      shard_path, repository_relative_path = args
+      shard_name, repository_relative_path = args
+      shard_path = Gitlab.config.repositories.storages.fetch(shard_name).legacy_disk_path
       # We can't leave the hooks in place after a fork, as those would fail in tests
       # The "internal" API is not available
       FileUtils.rm_rf(File.join(shard_path, repository_relative_path, 'hooks'))

spec/support/test_env.rb

@@ -62,6 +62,7 @@ module TestEnv
   }.freeze
 
   TMP_TEST_PATH = Rails.root.join('tmp', 'tests', '**')
+  REPOS_STORAGE = 'default'.freeze
 
   # Test environment
   #
@@ -225,7 +226,7 @@ module TestEnv
   end
 
   def repos_path
-    Gitlab.config.repositories.storages.default.legacy_disk_path
+    Gitlab.config.repositories.storages[REPOS_STORAGE].legacy_disk_path
   end
 
   def backup_path

spec/workers/repository_fork_worker_spec.rb

@@ -9,22 +9,19 @@ describe RepositoryForkWorker do
 
   describe "#perform" do
     let(:project) { create(:project, :repository) }
-    let(:fork_project) { create(:project, :repository, :import_scheduled, forked_from_project: project) }
     let(:shell) { Gitlab::Shell.new }
+    let(:fork_project) { create(:project, :repository, :import_scheduled, forked_from_project: project) }
 
+    shared_examples 'RepositoryForkWorker performing' do
       before do
         allow(subject).to receive(:gitlab_shell).and_return(shell)
       end
 
-    def perform!
-      subject.perform(fork_project.id, '/test/path', project.disk_path)
-    end
-
       def expect_fork_repository
         expect(shell).to receive(:fork_repository).with(
-        '/test/path',
+          'default',
           project.disk_path,
-        fork_project.repository_storage_path,
+          fork_project.repository_storage,
           fork_project.disk_path
         )
       end
@@ -75,4 +72,28 @@ describe RepositoryForkWorker do
         expect { perform! }.to raise_error(StandardError, error_message)
       end
     end
+
+    context 'only project ID passed' do
+      def perform!
+        subject.perform(fork_project.id)
+      end
+
+      it_behaves_like 'RepositoryForkWorker performing'
+    end
+
+    context 'project ID, storage and repo paths passed' do
+      def perform!
+        subject.perform(fork_project.id, TestEnv.repos_path, project.disk_path)
+      end
+
+      it_behaves_like 'RepositoryForkWorker performing'
+
+      it 'logs a message about forking with old-style arguments' do
+        allow(Rails.logger).to receive(:info).with(anything) # To compensate for other logs
+        expect(Rails.logger).to receive(:info).with("Project #{fork_project.id} is being forked using old-style arguments.")
+
+        perform!
+      end
+    end
+  end
 end