Merge branch 'zj-gitaly-namespace-service' into 'master'

Gitaly namespace service enabled for GitLab

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

app/models/project.rb

@@ -1034,6 +1034,8 @@ class Project < ActiveRecord::Base
     end
 
     true
+  rescue GRPC::Internal # if the path is too long
+    false
   end
 
   def create_repository(force: false)

lib/gitlab/git/repository.rb

@@ -1112,6 +1112,8 @@ module Gitlab
         raise NoRepository.new(e)
       rescue GRPC::BadStatus => e
         raise CommandError.new(e)
+      rescue GRPC::InvalidArgument => e
+        raise ArgumentError.new(e)
       end
 
       private

lib/gitlab/gitaly_client/namespace_service.rb

+module Gitlab
+  module GitalyClient
+    class NamespaceService
+      def initialize(storage)
+        @storage = storage
+      end
+
+      def exists?(name)
+        request = Gitaly::NamespaceExistsRequest.new(storage_name: @storage, name: name)
+
+        gitaly_client_call(:namespace_exists, request).exists
+      end
+
+      def add(name)
+        request = Gitaly::AddNamespaceRequest.new(storage_name: @storage, name: name)
+
+        gitaly_client_call(:add_namespace, request)
+      end
+
+      def remove(name)
+        request = Gitaly::RemoveNamespaceRequest.new(storage_name: @storage, name: name)
+
+        gitaly_client_call(:remove_namespace, request)
+      end
+
+      def rename(from, to)
+        request = Gitaly::RenameNamespaceRequest.new(storage_name: @storage, from: from, to: to)
+
+        gitaly_client_call(:rename_namespace, request)
+      end
+
+      private
+
+      def gitaly_client_call(type, request)
+        GitalyClient.call(@storage, :namespace_service, type, request)
+      end
+    end
+  end
+end

lib/gitlab/shell.rb

@@ -222,10 +222,18 @@ module Gitlab
     #
     # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385
     def add_namespace(storage, name)
+      Gitlab::GitalyClient.migrate(:add_namespace) do |enabled|
+        if enabled
+          gitaly_namespace_client(storage).add(name)
+        else
           path = full_path(storage, name)
           FileUtils.mkdir_p(path, mode: 0770) unless exists?(storage, name)
+        end
+      end
     rescue Errno::EEXIST => e
       Rails.logger.warn("Directory exists as a file: #{e} at: #{path}")
+    rescue GRPC::InvalidArgument => e
+      raise ArgumentError, e.message
     end
 
     # Remove directory from repositories storage
@@ -236,8 +244,16 @@ module Gitlab
     #
     # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385
     def rm_namespace(storage, name)
+      Gitlab::GitalyClient.migrate(:remove_namespace) do |enabled|
+        if enabled
+          gitaly_namespace_client(storage).remove(name)
+        else
           FileUtils.rm_r(full_path(storage, name), force: true)
         end
+      end
+    rescue GRPC::InvalidArgument => e
+      raise ArgumentError, e.message
+    end
 
     # Move namespace directory inside repositories storage
     #
@@ -246,10 +262,18 @@ module Gitlab
     #
     # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385
     def mv_namespace(storage, old_name, new_name)
+      Gitlab::GitalyClient.migrate(:rename_namespace) do |enabled|
+        if enabled
+          gitaly_namespace_client(storage).rename(old_name, new_name)
+        else
           return false if exists?(storage, new_name) || !exists?(storage, old_name)
 
           FileUtils.mv(full_path(storage, old_name), full_path(storage, new_name))
         end
+      end
+    rescue GRPC::InvalidArgument
+      false
+    end
 
     def url_to_repo(path)
       Gitlab.config.gitlab_shell.ssh_path_prefix + "#{path}.git"
@@ -272,8 +296,14 @@ module Gitlab
     #
     # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385
     def exists?(storage, dir_name)
+      Gitlab::GitalyClient.migrate(:namespace_exists) do |enabled|
+        if enabled
+          gitaly_namespace_client(storage).exists?(dir_name)
+        else
           File.exist?(full_path(storage, dir_name))
         end
+      end
+    end
 
     protected
 
@@ -349,6 +379,14 @@ module Gitlab
       Bundler.with_original_env { Popen.popen(cmd, nil, vars) }
     end
 
+    def gitaly_namespace_client(storage_path)
+      storage, _value = Gitlab.config.repositories.storages.find do |storage, value|
+        value['path'] == storage_path
+      end
+
+      Gitlab::GitalyClient::NamespaceService.new(storage)
+    end
+
     def gitaly_migrate(method, &block)
       Gitlab::GitalyClient.migrate(method, &block)
     rescue GRPC::NotFound, GRPC::BadStatus => e

lib/tasks/gitlab/gitaly.rake

@@ -50,6 +50,8 @@ namespace :gitlab do
     # only generate a configuration for the most common and simplest case: when
     # we have exactly one Gitaly process and we are sure it is running locally
     # because it uses a Unix socket.
+    # For development and testing purposes, an extra storage is added to gitaly,
+    # which is not known to Rails, but must be explicitly stubbed.
     def gitaly_configuration_toml(gitaly_ruby: true)
       storages = []
       address = nil
@@ -67,6 +69,11 @@ namespace :gitlab do
 
         storages << { name: key, path: val['path'] }
       end
+
+      if Rails.env.test?
+        storages << { name: 'test_second_storage', path: Rails.root.join('tmp', 'tests', 'second_storage').to_s }
+      end
+
       config = { socket_path: address.sub(%r{\Aunix:}, ''), storage: storages }
       config[:auth] = { token: 'secret' } if Rails.env.test?
       config[:'gitaly-ruby'] = { dir: File.join(Dir.pwd, 'ruby') } if gitaly_ruby

spec/controllers/admin/users_controller_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Admin::UsersController do
   let(:user) { create(:user) }
-  let(:admin) { create(:admin) }
+  set(:admin) { create(:admin) }
 
   before do
     sign_in(admin)

spec/controllers/projects_controller_spec.rb

@@ -140,7 +140,8 @@ describe ProjectsController do
     end
 
     context 'when the storage is not available', broken_storage: true do
-      let(:project) { create(:project, :broken_storage) }
+      set(:project) { create(:project, :broken_storage) }
+
       before do
         project.add_developer(user)
         sign_in(user)

spec/lib/gitlab/shell_spec.rb

@@ -15,10 +15,6 @@ describe Gitlab::Shell 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 :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") }
 
@@ -363,4 +359,52 @@ describe Gitlab::Shell do
       end
     end
   end
+
+  describe 'namespace actions' do
+    subject { described_class.new }
+    let(:storage_path) { Gitlab.config.repositories.storages.default.path }
+
+    describe '#add_namespace' do
+      it 'creates a namespace' do
+        subject.add_namespace(storage_path, "mepmep")
+
+        expect(subject.exists?(storage_path, "mepmep")).to be(true)
+      end
+    end
+
+    describe '#exists?' do
+      context 'when the namespace does not exist' do
+        it 'returns false' do
+          expect(subject.exists?(storage_path, "non-existing")).to be(false)
+        end
+      end
+
+      context 'when the namespace exists' do
+        it 'returns true' do
+          subject.add_namespace(storage_path, "mepmep")
+
+          expect(subject.exists?(storage_path, "mepmep")).to be(true)
+        end
+      end
+    end
+
+    describe '#remove' do
+      it 'removes the namespace' do
+        subject.add_namespace(storage_path, "mepmep")
+        subject.rm_namespace(storage_path, "mepmep")
+
+        expect(subject.exists?(storage_path, "mepmep")).to be(false)
+      end
+    end
+
+    describe '#mv_namespace' do
+      it 'renames the namespace' do
+        subject.add_namespace(storage_path, "mepmep")
+        subject.mv_namespace(storage_path, "mepmep", "2mep")
+
+        expect(subject.exists?(storage_path, "mepmep")).to be(false)
+        expect(subject.exists?(storage_path, "2mep")).to be(true)
+      end
+    end
+  end
 end

spec/models/namespace_spec.rb

@@ -152,22 +152,24 @@ describe Namespace do
   end
 
   describe '#move_dir' do
+    let(:namespace) { create(:namespace) }
+    let!(:project) { create(:project_empty_repo, namespace: namespace) }
+
     before do
-      @namespace = create :namespace
-      @project = create(:project_empty_repo, namespace: @namespace)
-      allow(@namespace).to receive(:path_changed?).and_return(true)
+      allow(namespace).to receive(:path_changed?).and_return(true)
     end
 
     it "raises error when directory exists" do
-      expect { @namespace.move_dir }.to raise_error("namespace directory cannot be moved")
+      expect { namespace.move_dir }.to raise_error("namespace directory cannot be moved")
     end
 
     it "moves dir if path changed" do
-      new_path = @namespace.full_path + "_new"
-      allow(@namespace).to receive(:full_path_was).and_return(@namespace.full_path)
-      allow(@namespace).to receive(:full_path).and_return(new_path)
-      expect(@namespace).to receive(:remove_exports!)
-      expect(@namespace.move_dir).to be_truthy
+      new_path = namespace.full_path + "_new"
+
+      allow(namespace).to receive(:full_path_was).and_return(namespace.full_path)
+      allow(namespace).to receive(:full_path).and_return(new_path)
+      expect(namespace).to receive(:remove_exports!)
+      expect(namespace.move_dir).to be_truthy
     end
 
     context "when any project has container images" do
@@ -177,14 +179,14 @@ describe Namespace do
         stub_container_registry_config(enabled: true)
         stub_container_registry_tags(repository: :any, tags: ['tag'])
 
-        create(:project, namespace: @namespace, container_repositories: [container_repository])
+        create(:project, namespace: namespace, container_repositories: [container_repository])
 
-        allow(@namespace).to receive(:path_was).and_return(@namespace.path)
-        allow(@namespace).to receive(:path).and_return('new_path')
+        allow(namespace).to receive(:path_was).and_return(namespace.path)
+        allow(namespace).to receive(:path).and_return('new_path')
       end
 
       it 'raises an error about not movable project' do
-        expect { @namespace.move_dir }.to raise_error(/Namespace cannot be moved/)
+        expect { namespace.move_dir }.to raise_error(/Namespace cannot be moved/)
       end
     end
 

spec/models/project_spec.rb

@@ -421,20 +421,10 @@ describe Project do
   end
 
   describe '#repository_storage_path' do
-    let(:project) { create(:project, repository_storage: 'custom') }
-
-    before do
-      FileUtils.mkdir('tmp/tests/custom_repositories')
-      storages = { 'custom' => { 'path' => '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
+    let(:project) { create(:project) }
 
     it 'returns the repository storage path' do
-      expect(project.repository_storage_path).to eq('tmp/tests/custom_repositories')
+      expect(Dir.exist?(project.repository_storage_path)).to be(true)
     end
   end
 

spec/models/repository_spec.rb

@@ -634,6 +634,10 @@ describe Repository do
   end
 
   describe '#fetch_ref' do
+    # Setting the var here, sidesteps the stub that makes gitaly raise an error
+    # before the actual test call
+    set(:broken_repository) { create(:project, :broken_storage).repository }
+
     describe 'when storage is broken', broken_storage: true  do
       it 'should raise a storage error' do
         expect_to_raise_storage_error do

spec/tasks/gitlab/backup_rake_spec.rb

@@ -224,17 +224,20 @@ describe 'gitlab:app namespace rake task' do
     end
 
     context 'multiple repository storages' do
+      let(:gitaly_address) { Gitlab.config.repositories.storages.default.gitaly_address }
+      let(:storages) do
+        {
+          'default' => { 'path' => Settings.absolute('tmp/tests/default_storage'), 'gitaly_address' => gitaly_address  },
+          'test_second_storage' => { 'path' => Settings.absolute('tmp/tests/custom_storage'), 'gitaly_address' => gitaly_address }
+        }
+      end
+
       let(:project_a) { create(:project, :repository, repository_storage: 'default') }
-      let(:project_b) { create(:project, :repository, repository_storage: 'custom') }
+      let(:project_b) { create(:project, :repository, repository_storage: 'test_second_storage') }
 
       before do
         FileUtils.mkdir('tmp/tests/default_storage')
         FileUtils.mkdir('tmp/tests/custom_storage')
-        gitaly_address = Gitlab.config.repositories.storages.default.gitaly_address
-        storages = {
-          'default' => { 'path' => Settings.absolute('tmp/tests/default_storage'), 'gitaly_address' => gitaly_address  },
-          'custom' => { 'path' => Settings.absolute('tmp/tests/custom_storage'), 'gitaly_address' => gitaly_address }
-        }
         allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
 
         # Create the projects now, after mocking the settings but before doing the backup
@@ -253,7 +256,7 @@ describe 'gitlab:app namespace rake task' do
       after do
         FileUtils.rm_rf('tmp/tests/default_storage')
         FileUtils.rm_rf('tmp/tests/custom_storage')
-        FileUtils.rm(@backup_tar)
+        FileUtils.rm(@backup_tar) if @backup_tar
       end
 
       it 'includes repositories in all repository storages' do