Merge branch 'gitaly-reuse-stubs' into 'master'

app/models/repository.rb

@@ -1163,8 +1163,6 @@ class Repository
     @project.repository_storage_path
   end
 
-  delegate :gitaly_channel, :gitaly_repository, to: :raw_repository
-
   def initialize_raw_repository
     Gitlab::Git::Repository.new(project.repository_storage, path_with_namespace + '.git')
   end

config/initializers/8_gitaly.rb

 require 'uri'
 
-# Make sure we initialize our Gitaly channels before Sidekiq starts multi-threaded execution.
 if Gitlab.config.gitaly.enabled || Rails.env.test?
-  Gitlab::GitalyClient.configure_channels
+  Gitlab.config.repositories.storages.keys.each do |storage|
+    # Force validation of each address
+    Gitlab::GitalyClient.address(storage)
+  end
 end

lib/gitlab/git/repository.rb

@@ -27,13 +27,15 @@ module Gitlab
       # Rugged repo object
       attr_reader :rugged
 
+      attr_reader :storage
+
       # 'path' must be the path to a _bare_ git repository, e.g.
       # /path/to/my-repo.git
-      def initialize(repository_storage, relative_path)
-        @repository_storage = repository_storage
+      def initialize(storage, relative_path)
+        @storage = storage
         @relative_path = relative_path
 
-        storage_path = Gitlab.config.repositories.storages[@repository_storage]['path']
+        storage_path = Gitlab.config.repositories.storages[@storage]['path']
         @path = File.join(storage_path, @relative_path)
         @name = @relative_path.split("/").last
         @attributes = Gitlab::Git::Attributes.new(path)
@@ -965,11 +967,7 @@ module Gitlab
       end
 
       def gitaly_repository
-        Gitlab::GitalyClient::Util.repository(@repository_storage, @relative_path)
-      end
-
-      def gitaly_channel
-        Gitlab::GitalyClient.get_channel(@repository_storage)
+        Gitlab::GitalyClient::Util.repository(@storage, @relative_path)
       end
 
       private

lib/gitlab/gitaly_client.rb

@@ -4,56 +4,42 @@ module Gitlab
   module GitalyClient
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'.freeze
 
-    # This function is not thread-safe because it updates Hashes in instance variables.
-    def self.configure_channels
-      @addresses = {}
-      @channels = {}
-      Gitlab.config.repositories.storages.each do |name, params|
-        address = params['gitaly_address']
-        unless address.present?
-          raise "storage #{name.inspect} is missing a gitaly_address"
-        end
+    MUTEX = Mutex.new
+    private_constant :MUTEX
 
-        unless URI(address).scheme.in?(%w(tcp unix))
-          raise "Unsupported Gitaly address: #{address.inspect} does not use URL scheme 'tcp' or 'unix'"
+    def self.stub(name, storage)
+      MUTEX.synchronize do
+        @stubs ||= {}
+        @stubs[storage] ||= {}
+        @stubs[storage][name] ||= begin
+          klass = Gitaly.const_get(name.to_s.camelcase.to_sym).const_get(:Stub)
+          addr = address(storage)
+          addr = addr.sub(%r{^tcp://}, '') if URI(addr).scheme == 'tcp'
+          klass.new(addr, :this_channel_is_insecure)
         end
-
-        @addresses[name] = address
-        @channels[name] = new_channel(address)
       end
     end
 
-    def self.new_channel(address)
-      address = address.sub(%r{^tcp://}, '') if URI(address).scheme == 'tcp'
-      # NOTE: When Gitaly runs on a Unix socket, permissions are
-      # handled using the file system and no additional authentication is
-      # required (therefore the :this_channel_is_insecure flag)
-      # TODO: Add authentication support when Gitaly is running on a TCP socket.
-      GRPC::Core::Channel.new(address, {}, :this_channel_is_insecure)
+    def self.clear_stubs!
+      MUTEX.synchronize do
+        @stubs = nil
       end
-
-    def self.get_channel(storage)
-      if !Rails.env.production? && @channels.nil?
-        # In development mode the Rails auto-loader may reset the instance
-        # variables of this class. What we do here is not thread-safe. In normal
-        # circumstances (including production) these instance variables have
-        # been initialized from config/initializers.
-        configure_channels
     end
 
-      @channels[storage]
+    def self.address(storage)
+      params = Gitlab.config.repositories.storages[storage]
+      raise "storage not found: #{storage.inspect}" if params.nil?
+
+      address = params['gitaly_address']
+      unless address.present?
+        raise "storage #{storage.inspect} is missing a gitaly_address"
       end
 
-    def self.get_address(storage)
-      if !Rails.env.production? && @addresses.nil?
-        # In development mode the Rails auto-loader may reset the instance
-        # variables of this class. What we do here is not thread-safe. In normal
-        # circumstances (including development) these instance variables have
-        # been initialized from config/initializers.
-        configure_channels
+      unless URI(address).scheme.in?(%w(tcp unix))
+        raise "Unsupported Gitaly address: #{address.inspect} does not use URL scheme 'tcp' or 'unix'"
       end
 
-      @addresses[storage]
+      address
     end
 
     def self.enabled?

lib/gitlab/gitaly_client/commit.rb

@@ -11,7 +11,7 @@ module Gitlab
       end
 
       def is_ancestor(ancestor_id, child_id)
-        stub = Gitaly::Commit::Stub.new(nil, nil, channel_override: @repository.gitaly_channel)
+        stub = GitalyClient.stub(:commit, @repository.storage)
         request = Gitaly::CommitIsAncestorRequest.new(
           repository: @gitaly_repo,
           ancestor_id: ancestor_id,
@@ -52,7 +52,7 @@ module Gitlab
       end
 
       def diff_service_stub
-        Gitaly::Diff::Stub.new(nil, nil, channel_override: @repository.gitaly_channel)
+        GitalyClient.stub(:diff, @repository.storage)
       end
     end
   end

lib/gitlab/gitaly_client/notifications.rb

@@ -6,7 +6,7 @@ module Gitlab
       # 'repository' is a Gitlab::Git::Repository
       def initialize(repository)
         @gitaly_repo = repository.gitaly_repository
-        @stub = Gitaly::Notifications::Stub.new(nil, nil, channel_override: repository.gitaly_channel)
+        @stub = GitalyClient.stub(:notifications, repository.storage)
       end
 
       def post_receive

lib/gitlab/gitaly_client/ref.rb

@@ -6,7 +6,7 @@ module Gitlab
       # 'repository' is a Gitlab::Git::Repository
       def initialize(repository)
         @gitaly_repo = repository.gitaly_repository
-        @stub = Gitaly::Ref::Stub.new(nil, nil, channel_override: repository.gitaly_channel)
+        @stub = GitalyClient.stub(:ref, repository.storage)
       end
 
       def default_branch_name

lib/gitlab/workhorse.rb

@@ -26,7 +26,7 @@ module Gitlab
         }
 
         if Gitlab.config.gitaly.enabled
-          address = Gitlab::GitalyClient.get_address(project.repository_storage)
+          address = Gitlab::GitalyClient.address(project.repository_storage)
           params[:Repository] = repository.gitaly_repository.to_h
 
           feature_enabled = case action.to_s

spec/lib/gitlab/gitaly_client_spec.rb

 require 'spec_helper'
 
 describe Gitlab::GitalyClient, lib: true do
-  describe '.new_channel' do
+  describe '.stub' do
+    before { described_class.clear_stubs! }
+
     context 'when passed a UNIX socket address' do
-      it 'passes the address as-is to GRPC::Core::Channel initializer' do
+      it 'passes the address as-is to GRPC' do
         address = 'unix:/tmp/gitaly.sock'
+        allow(Gitlab.config.repositories).to receive(:storages).and_return({
+          'default' => { 'gitaly_address' => address }
+        })
 
-        expect(GRPC::Core::Channel).to receive(:new).with(address, any_args)
+        expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args)
 
-        described_class.new_channel(address)
+        described_class.stub(:commit, 'default')
       end
     end
 
@@ -17,9 +22,13 @@ describe Gitlab::GitalyClient, lib: true do
         address = 'localhost:9876'
         prefixed_address = "tcp://#{address}"
 
-        expect(GRPC::Core::Channel).to receive(:new).with(address, any_args)
+        allow(Gitlab.config.repositories).to receive(:storages).and_return({
+          'default' => { 'gitaly_address' => prefixed_address }
+        })
+
+        expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args)
 
-        described_class.new_channel(prefixed_address)
+        described_class.stub(:commit, 'default')
       end
     end
   end

spec/lib/gitlab/workhorse_spec.rb

@@ -202,7 +202,7 @@ describe Gitlab::Workhorse, lib: true do
     context 'when Gitaly is enabled' do
       let(:gitaly_params) do
         {
-          GitalyAddress: Gitlab::GitalyClient.get_address('default')
+          GitalyAddress: Gitlab::GitalyClient.address('default')
         }
       end
 

spec/support/test_env.rb

@@ -120,7 +120,7 @@ module TestEnv
   end
 
   def setup_gitaly
-    socket_path = Gitlab::GitalyClient.get_address('default').sub(/\Aunix:/, '')
+    socket_path = Gitlab::GitalyClient.address('default').sub(/\Aunix:/, '')
     gitaly_dir = File.dirname(socket_path)
 
     unless File.directory?(gitaly_dir) || system('rake', "gitlab:gitaly:install[#{gitaly_dir}]")
@@ -133,7 +133,8 @@ module TestEnv
   def start_gitaly(gitaly_dir)
     gitaly_exec = File.join(gitaly_dir, 'gitaly')
     gitaly_config = File.join(gitaly_dir, 'config.toml')
-    @gitaly_pid = spawn(gitaly_exec, gitaly_config, [:out, :err] => '/dev/null')
+    log_file = Rails.root.join('log/gitaly-test.log').to_s
+    @gitaly_pid = spawn(gitaly_exec, gitaly_config, [:out, :err] => log_file)
   end
 
   def stop_gitaly

spec/tasks/gitlab/backup_rake_spec.rb

@@ -236,7 +236,6 @@ describe 'gitlab:app namespace rake task' do
           'custom' => { 'path' => Settings.absolute('tmp/tests/custom_storage'), 'gitaly_address' => gitaly_address }
         }
         allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
-        Gitlab::GitalyClient.configure_channels
 
         # Create the projects now, after mocking the settings but before doing the backup
         project_a