Refactor repository paths handling to allow multiple git mount points

13 files changed, 210 insertions, 73 deletions

diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb
index 09e0bbfd00b..604204cca0a 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -123,11 +123,17 @@ describe ProjectsHelper do
   end
 
   describe '#sanitized_import_error' do
+    let(:project) { create(:project) }
+
+    before do
+      allow(project).to receive(:repository_storage_path).and_return('/base/repo/path')
+    end
+
     it 'removes the repo path' do
-      repo = File.join(Gitlab.config.gitlab_shell.repos_path, '/namespace/test.git')
+      repo = '/base/repo/path/namespace/test.git'
       import_error = "Could not clone #{repo}\n"
 
-      expect(sanitize_repo_path(import_error)).to eq('Could not clone [REPOS PATH]/namespace/test.git')
+      expect(sanitize_repo_path(project, import_error)).to eq('Could not clone [REPOS PATH]/namespace/test.git')
     end
   end
 end
diff --git a/spec/initializers/6_validations_spec.rb b/spec/initializers/6_validations_spec.rb
new file mode 100644
index 00000000000..5178bd130f4
--- /dev/null
+++ b/spec/initializers/6_validations_spec.rb
@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+describe '6_validations', lib: true do
+  context 'with correct settings' do
+    before do
+      mock_storages('foo' => '/a/b/c', 'bar' => 'a/b/d')
+    end
+
+    it 'passes through' do
+      expect { load_validations }.not_to raise_error
+    end
+  end
+
+  context 'with invalid storage names' do
+    before do
+      mock_storages('name with spaces' => '/a/b/c')
+    end
+
+    it 'throws an error' do
+      expect { load_validations }.to raise_error('"name with spaces" is not a valid storage name. Please fix this in your gitlab.yml before starting GitLab.')
+    end
+  end
+
+  context 'with nested storage paths' do
+    before do
+      mock_storages('foo' => '/a/b/c', 'bar' => '/a/b/c/d')
+    end
+
+    it 'throws an error' do
+      expect { load_validations }.to raise_error('bar is a nested path of foo. Nested paths are not supported for repository storages. Please fix this in your gitlab.yml before starting GitLab.')
+    end
+  end
+
+  def mock_storages(storages)
+    allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
+  end
+
+  def load_validations
+    load File.join(__dir__, '../../config/initializers/6_validations.rb')
+  end
+end
diff --git a/spec/lib/gitlab/backend/shell_spec.rb b/spec/lib/gitlab/backend/shell_spec.rb
index fd869f48b5c..e15f13f985b 100644
--- a/spec/lib/gitlab/backend/shell_spec.rb
+++ b/spec/lib/gitlab/backend/shell_spec.rb
@@ -13,6 +13,11 @@ describe Gitlab::Shell, lib: true do
   it { is_expected.to respond_to :add_repository }
   it { is_expected.to respond_to :remove_repository }
   it { is_expected.to respond_to :fork_repository }
+  it { is_expected.to respond_to :gc }
+  it { is_expected.to respond_to :add_namespace }
+  it { is_expected.to respond_to :rm_namespace }
+  it { is_expected.to respond_to :mv_namespace }
+  it { is_expected.to respond_to :exists? }
 
   it { expect(gitlab_shell.url_to_repo('diaspora')).to eq(Gitlab.config.gitlab_shell.ssh_path_prefix + "diaspora.git") }
 
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 4e68ac5e63a..cbea407f9bf 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -57,6 +57,7 @@ describe Namespace, models: true do
   describe :move_dir do
     before do
       @namespace = create :namespace
+      @project = create :project, namespace: @namespace
       allow(@namespace).to receive(:path_changed?).and_return(true)
     end
 
@@ -87,8 +88,13 @@ describe Namespace, models: true do
   end
 
   describe :rm_dir do
-    it "should remove dir" do
-      expect(namespace.rm_dir).to be_truthy
+    let!(:project) { create(:project, namespace: namespace) }
+    let!(:path) { File.join(Gitlab.config.repositories.storages.default, namespace.path) }
+
+    before { namespace.destroy }
+
+    it "should remove its dirs when deleted" do
+      expect(File.exist?(path)).to be(false)
     end
   end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index d305cd9ff1e..ee1142fa3aa 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -56,6 +56,7 @@ describe Project, models: true do
     it { is_expected.to validate_length_of(:description).is_within(0..2000) }
     it { is_expected.to validate_presence_of(:creator) }
     it { is_expected.to validate_presence_of(:namespace) }
+    it { is_expected.to validate_presence_of(:repository_storage) }
 
     it 'should not allow new projects beyond user limits' do
       project2 = build(:project)
@@ -84,6 +85,20 @@ describe Project, models: true do
         end
       end
     end
+
+    context 'repository storages inclussion' do
+      let(:project2) { build(:project, repository_storage: 'missing') }
+
+      before do
+        storages = { 'custom' => 'tmp/tests/custom_repositories' }
+        allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
+      end
+
+      it "should not allow repository storages that don't match a label in the configuration" do
+        expect(project2).not_to be_valid
+        expect(project2.errors[:repository_storage].first).to match(/is not included in the list/)
+      end
+    end
   end
 
   describe 'default_scope' do
@@ -131,6 +146,24 @@ describe Project, models: true do
     end
   end
 
+  describe '#repository_storage_path' do
+    let(:project) { create(:project, repository_storage: 'custom') }
+
+    before do
+      FileUtils.mkdir('tmp/tests/custom_repositories')
+      storages = { 'custom' => 'tmp/tests/custom_repositories' }
+      allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
+    end
+
+    after do
+      FileUtils.rm_rf('tmp/tests/custom_repositories')
+    end
+
+    it 'returns the repository storage path' do
+      expect(project.repository_storage_path).to eq('tmp/tests/custom_repositories')
+    end
+  end
+
   it 'should return valid url to repo' do
     project = Project.new(path: 'somewhere')
     expect(project.url_to_repo).to eq(Gitlab.config.gitlab_shell.ssh_path_prefix + 'somewhere.git')
@@ -729,12 +762,12 @@ describe Project, models: true do
 
       expect(gitlab_shell).to receive(:mv_repository).
         ordered.
-        with("#{ns}/foo", "#{ns}/#{project.path}").
+        with(project.repository_storage_path, "#{ns}/foo", "#{ns}/#{project.path}").
         and_return(true)
 
       expect(gitlab_shell).to receive(:mv_repository).
         ordered.
-        with("#{ns}/foo.wiki", "#{ns}/#{project.path}.wiki").
+        with(project.repository_storage_path, "#{ns}/foo.wiki", "#{ns}/#{project.path}.wiki").
         and_return(true)
 
       expect_any_instance_of(SystemHooksService).
@@ -826,7 +859,7 @@ describe Project, models: true do
     context 'using a regular repository' do
       it 'creates the repository' do
         expect(shell).to receive(:add_repository).
-          with(project.path_with_namespace).
+          with(project.repository_storage_path, project.path_with_namespace).
           and_return(true)
 
         expect(project.repository).to receive(:after_create)
@@ -836,7 +869,7 @@ describe Project, models: true do
 
       it 'adds an error if the repository could not be created' do
         expect(shell).to receive(:add_repository).
-          with(project.path_with_namespace).
+          with(project.repository_storage_path, project.path_with_namespace).
           and_return(false)
 
         expect(project.repository).not_to receive(:after_create)
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 437c89c3577..fcea45f19ba 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -72,6 +72,7 @@ describe API::API, api: true  do
 
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
+          expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
         end
       end
 
@@ -81,6 +82,7 @@ describe API::API, api: true  do
 
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
+          expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
         end
       end
     end
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb
index afa89b84175..eca8ddd8ea4 100644
--- a/spec/services/destroy_group_service_spec.rb
+++ b/spec/services/destroy_group_service_spec.rb
@@ -23,8 +23,8 @@ describe DestroyGroupService, services: true do
         Sidekiq::Testing.inline! { destroy_group(group, user) }
       end
 
-      it { expect(gitlab_shell.exists?(group.path)).to be_falsey }
-      it { expect(gitlab_shell.exists?(remove_path)).to be_falsey }
+      it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+      it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
     end
 
     context 'Sidekiq fake' do
@@ -33,8 +33,8 @@ describe DestroyGroupService, services: true do
         Sidekiq::Testing.fake! { destroy_group(group, user) }
       end
 
-      it { expect(gitlab_shell.exists?(group.path)).to be_falsey }
-      it { expect(gitlab_shell.exists?(remove_path)).to be_truthy }
+      it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+      it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
     end
   end
 
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index 4c5ced7e746..bd4dc6a0f79 100644
--- a/spec/services/projects/housekeeping_service_spec.rb
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -12,7 +12,7 @@ describe Projects::HousekeepingService do
 
     it 'enqueues a sidekiq job' do
       expect(subject).to receive(:try_obtain_lease).and_return(true)
-      expect(GitlabShellOneShotWorker).to receive(:perform_async).with(:gc, project.path_with_namespace)
+      expect(GitlabShellOneShotWorker).to receive(:perform_async).with(:gc, project.repository_storage_path, project.path_with_namespace)
 
       subject.execute
       expect(project.pushes_since_gc).to eq(0)
diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb
index 068c9a1219c..d5d4d7c56ef 100644
--- a/spec/services/projects/import_service_spec.rb
+++ b/spec/services/projects/import_service_spec.rb
@@ -36,7 +36,7 @@ describe Projects::ImportService, services: true do
       end
 
       it 'succeeds if repository import is successfully' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
 
         result = subject.execute
 
@@ -44,7 +44,7 @@ describe Projects::ImportService, services: true do
       end
 
       it 'fails if repository import fails' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
 
         result = subject.execute
 
@@ -64,7 +64,7 @@ describe Projects::ImportService, services: true do
       end
 
       it 'succeeds if importer succeeds' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
         expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true)
 
         result = subject.execute
@@ -74,7 +74,7 @@ describe Projects::ImportService, services: true do
 
       it 'flushes various caches' do
         expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).
-          with(project.path_with_namespace, project.import_url).
+          with(project.repository_storage_path, project.path_with_namespace, project.import_url).
           and_return(true)
 
         expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).
@@ -90,7 +90,7 @@ describe Projects::ImportService, services: true do
       end
 
       it 'fails if importer fails' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
         expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false)
 
         result = subject.execute
@@ -100,7 +100,7 @@ describe Projects::ImportService, services: true do
       end
 
       it 'fails if importer raise an error' do
-        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.path_with_namespace, project.import_url).and_return(true)
+        expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true)
         expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API'))
 
         result = subject.execute
diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb
index 426bf53f198..be5331e4770 100644
--- a/spec/support/test_env.rb
+++ b/spec/support/test_env.rb
@@ -80,8 +80,9 @@ module TestEnv
   end
 
   def setup_gitlab_shell
-    unless File.directory?(Rails.root.join(*%w(tmp tests gitlab-shell)))
-      `rake gitlab:shell:install`
+    unless File.directory?(Gitlab.config.gitlab_shell.path)
+      # TODO: Remove `[shards]` when gitlab-shell v3.1.0 is published
+      `rake gitlab:shell:install[shards]`
     end
   end
 
@@ -127,14 +128,14 @@ module TestEnv
 
   def copy_repo(project)
     base_repo_path = File.expand_path(factory_repo_path_bare)
-    target_repo_path = File.expand_path(repos_path + "/#{project.namespace.path}/#{project.path}.git")
+    target_repo_path = File.expand_path(project.repository_storage_path + "/#{project.namespace.path}/#{project.path}.git")
     FileUtils.mkdir_p(target_repo_path)
     FileUtils.cp_r("#{base_repo_path}/.", target_repo_path)
     FileUtils.chmod_R 0755, target_repo_path
   end
 
   def repos_path
-    Gitlab.config.gitlab_shell.repos_path
+    Gitlab.config.repositories.storages.default
   end
 
   def backup_path
@@ -143,7 +144,7 @@ module TestEnv
 
   def copy_forked_repo_with_submodules(project)
     base_repo_path = File.expand_path(forked_repo_path_bare)
-    target_repo_path = File.expand_path(repos_path + "/#{project.namespace.path}/#{project.path}.git")
+    target_repo_path = File.expand_path(project.repository_storage_path + "/#{project.namespace.path}/#{project.path}.git")
     FileUtils.mkdir_p(target_repo_path)
     FileUtils.cp_r("#{base_repo_path}/.", target_repo_path)
     FileUtils.chmod_R 0755, target_repo_path
diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb
index 25da0917134..02308530d13 100644
--- a/spec/tasks/gitlab/backup_rake_spec.rb
+++ b/spec/tasks/gitlab/backup_rake_spec.rb
@@ -98,67 +98,107 @@ describe 'gitlab:app namespace rake task' do
       @backup_tar = tars_glob.first
     end
 
-    before do
-      create_backup
-    end
-
-    after do
-      FileUtils.rm(@backup_tar)
-    end
+    context 'tar creation' do
+      before do
+        create_backup
+      end
 
-    context 'archive file permissions' do
-      it 'should set correct permissions on the tar file' do
-        expect(File.exist?(@backup_tar)).to be_truthy
-        expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100600')
+      after do
+        FileUtils.rm(@backup_tar)
       end
 
-      context 'with custom archive_permissions' do
-        before do
-          allow(Gitlab.config.backup).to receive(:archive_permissions).and_return(0651)
-          # We created a backup in a before(:all) so it got the default permissions.
-          # We now need to do some work to create a _new_ backup file using our stub.
-          FileUtils.rm(@backup_tar)
-          create_backup
+      context 'archive file permissions' do
+        it 'should set correct permissions on the tar file' do
+          expect(File.exist?(@backup_tar)).to be_truthy
+          expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100600')
         end
 
-        it 'uses the custom permissions' do
-          expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100651')
+        context 'with custom archive_permissions' do
+          before do
+            allow(Gitlab.config.backup).to receive(:archive_permissions).and_return(0651)
+            # We created a backup in a before(:all) so it got the default permissions.
+            # We now need to do some work to create a _new_ backup file using our stub.
+            FileUtils.rm(@backup_tar)
+            create_backup
+          end
+
+          it 'uses the custom permissions' do
+            expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100651')
+          end
         end
       end
-    end
 
-    it 'should set correct permissions on the tar contents' do
-      tar_contents, exit_status = Gitlab::Popen.popen(
-        %W{tar -tvf #{@backup_tar} db uploads.tar.gz repositories builds.tar.gz artifacts.tar.gz lfs.tar.gz registry.tar.gz}
-      )
-      expect(exit_status).to eq(0)
-      expect(tar_contents).to match('db/')
-      expect(tar_contents).to match('uploads.tar.gz')
-      expect(tar_contents).to match('repositories/')
-      expect(tar_contents).to match('builds.tar.gz')
-      expect(tar_contents).to match('artifacts.tar.gz')
-      expect(tar_contents).to match('lfs.tar.gz')
-      expect(tar_contents).to match('registry.tar.gz')
-      expect(tar_contents).not_to match(/^.{4,9}[rwx].* (database.sql.gz|uploads.tar.gz|repositories|builds.tar.gz|artifacts.tar.gz|registry.tar.gz)\/$/)
-    end
+      it 'should set correct permissions on the tar contents' do
+        tar_contents, exit_status = Gitlab::Popen.popen(
+          %W{tar -tvf #{@backup_tar} db uploads.tar.gz repositories builds.tar.gz artifacts.tar.gz lfs.tar.gz registry.tar.gz}
+        )
+        expect(exit_status).to eq(0)
+        expect(tar_contents).to match('db/')
+        expect(tar_contents).to match('uploads.tar.gz')
+        expect(tar_contents).to match('repositories/')
+        expect(tar_contents).to match('builds.tar.gz')
+        expect(tar_contents).to match('artifacts.tar.gz')
+        expect(tar_contents).to match('lfs.tar.gz')
+        expect(tar_contents).to match('registry.tar.gz')
+        expect(tar_contents).not_to match(/^.{4,9}[rwx].* (database.sql.gz|uploads.tar.gz|repositories|builds.tar.gz|artifacts.tar.gz|registry.tar.gz)\/$/)
+      end
 
-    it 'should delete temp directories' do
-      temp_dirs = Dir.glob(
-        File.join(Gitlab.config.backup.path, '{db,repositories,uploads,builds,artifacts,lfs,registry}')
-      )
+      it 'should delete temp directories' do
+        temp_dirs = Dir.glob(
+          File.join(Gitlab.config.backup.path, '{db,repositories,uploads,builds,artifacts,lfs,registry}')
+        )
+
+        expect(temp_dirs).to be_empty
+      end
 
-      expect(temp_dirs).to be_empty
+      context 'registry disabled' do
+        let(:enable_registry) { false }
+
+        it 'should not create registry.tar.gz' do
+          tar_contents, exit_status = Gitlab::Popen.popen(
+            %W{tar -tvf #{@backup_tar}}
+          )
+          expect(exit_status).to eq(0)
+          expect(tar_contents).not_to match('registry.tar.gz')
+        end
+      end
     end
 
-    context 'registry disabled' do
-      let(:enable_registry) { false }
+    context 'multiple repository storages' do
+      let(:project_a) { create(:project, repository_storage: 'default') }
+      let(:project_b) { create(:project, repository_storage: 'custom') }
+
+      before do
+        FileUtils.mkdir('tmp/tests/default_storage')
+        FileUtils.mkdir('tmp/tests/custom_storage')
+        storages = {
+          'default' => 'tmp/tests/default_storage',
+          'custom' => 'tmp/tests/custom_storage'
+        }
+        allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
+
+        # Create the projects now, after mocking the settings but before doing the backup
+        project_a
+        project_b
+
+        # We only need a backup of the repositories for this test
+        ENV["SKIP"] = "db,uploads,builds,artifacts,lfs,registry"
+        create_backup
+      end
+
+      after do
+        FileUtils.rm_rf('tmp/tests/default_storage')
+        FileUtils.rm_rf('tmp/tests/custom_storage')
+        FileUtils.rm(@backup_tar)
+      end
 
-      it 'should not create registry.tar.gz' do
+      it 'should include repositories in all repository storages' do
         tar_contents, exit_status = Gitlab::Popen.popen(
-          %W{tar -tvf #{@backup_tar}}
+          %W{tar -tvf #{@backup_tar} repositories}
         )
         expect(exit_status).to eq(0)
-        expect(tar_contents).not_to match('registry.tar.gz')
+        expect(tar_contents).to match("repositories/#{project_a.path_with_namespace}.bundle")
+        expect(tar_contents).to match("repositories/#{project_b.path_with_namespace}.bundle")
       end
     end
   end # backup_create task
diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb
index b8e73682c91..20b1a343c27 100644
--- a/spec/workers/post_receive_spec.rb
+++ b/spec/workers/post_receive_spec.rb
@@ -91,6 +91,6 @@ describe PostReceive do
   end
 
   def pwd(project)
-    File.join(Gitlab.config.gitlab_shell.repos_path, project.path_with_namespace)
+    File.join(Gitlab.config.repositories.storages.default, project.path_with_namespace)
   end
 end
diff --git a/spec/workers/repository_fork_worker_spec.rb b/spec/workers/repository_fork_worker_spec.rb
index 4ef05eb29d2..5f762282b5e 100644
--- a/spec/workers/repository_fork_worker_spec.rb
+++ b/spec/workers/repository_fork_worker_spec.rb
@@ -14,6 +14,7 @@ describe RepositoryForkWorker do
   describe "#perform" do
     it "creates a new repository from a fork" do
       expect(shell).to receive(:fork_repository).with(
+        project.repository_storage_path,
         project.path_with_namespace,
         fork_project.namespace.path
       ).and_return(true)
@@ -25,9 +26,11 @@ describe RepositoryForkWorker do
     end
 
     it 'flushes various caches' do
-      expect(shell).to receive(:fork_repository).
-        with(project.path_with_namespace, fork_project.namespace.path).
-        and_return(true)
+      expect(shell).to receive(:fork_repository).with(
+        project.repository_storage_path,
+        project.path_with_namespace,
+        fork_project.namespace.path
+      ).and_return(true)
 
       expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).
         and_call_original