diff --git a/app/models/repository.rb b/app/models/repository.rb index b1563bfba8ba7eb30e63078de3195ecbaef23766..9153e5ae5a82204f247b695923c6332fc0816a9a 100644 --- a/app/models/repository.rb +++ b/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 diff --git a/config/initializers/8_gitaly.rb b/config/initializers/8_gitaly.rb index 42ec7240b0fd339598a7b146cd2cb9931c9b4467..31c7c91d78fd313f12c4d78943383dd32d29c1e7 100644 --- a/config/initializers/8_gitaly.rb +++ b/config/initializers/8_gitaly.rb @@ -1,6 +1,8 @@ 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 diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 256318cb83300e9e69b4c8a3da7ecdcd542c684e..7a051444603082a431fc99beefac8026c3d614fb 100644 --- a/lib/gitlab/git/repository.rb +++ b/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 diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index c69676a1dac5e6130ef9e02d96f76c14c8d278d8..72466700c053548eff41224acf315a2bc163ab2d 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/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 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 + def self.address(storage) + params = Gitlab.config.repositories.storages[storage] + raise "storage not found: #{storage.inspect}" if params.nil? - @channels[storage] - end + 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? diff --git a/lib/gitlab/gitaly_client/commit.rb b/lib/gitlab/gitaly_client/commit.rb index 15c57420fb248d34a5eafb325d938faee24b706f..4491903d788226824ce4337d498b8e6b43270bc9 100644 --- a/lib/gitlab/gitaly_client/commit.rb +++ b/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 diff --git a/lib/gitlab/gitaly_client/notifications.rb b/lib/gitlab/gitaly_client/notifications.rb index a94a54883db4d168af246e2a4cff8ee1f89835da..719554eac52772acc12ed1fd3ad00f4526f7073a 100644 --- a/lib/gitlab/gitaly_client/notifications.rb +++ b/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 diff --git a/lib/gitlab/gitaly_client/ref.rb b/lib/gitlab/gitaly_client/ref.rb index f6c77ef1a3e50b09cf718e0736aa4e3f1e06c178..bf04e1fa50b9b088857355e72a34563312aeef21 100644 --- a/lib/gitlab/gitaly_client/ref.rb +++ b/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 diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 351e2b1059524ad02eaafccf252d18831f64ea2f..fe37e4da94f2000bc21910f3ba5fb39a497746c9 100644 --- a/lib/gitlab/workhorse.rb +++ b/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 diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb index 55fcf91fb6e9c258dc0b15e3100cc0d1eb0dbd3e..08ee0dff6b2c87ed03e938d910634cba020ffd91 100644 --- a/spec/lib/gitlab/gitaly_client_spec.rb +++ b/spec/lib/gitlab/gitaly_client_spec.rb @@ -1,14 +1,19 @@ 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 diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index 67b759f7dcd5267d4b4f8aeb04e93c626bdf52dc..fdbb55fc874b85886d6727bdfbf67e2a0dd8cc72 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/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 diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 8e31c26591b1c25af9f38ade2add1169b12d2b13..9bf9dc5d4b2410b5ae35980da262f1196348a1d8 100644 --- a/spec/support/test_env.rb +++ b/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 diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb index 4def113dd77f3979b3963ec42d25d9b8cd88a63f..0ff1a988a9e2d42655a73f16e12590870c82caef 100644 --- a/spec/tasks/gitlab/backup_rake_spec.rb +++ b/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