Commit 622b8e62 authored by Qingyu Zhao's avatar Qingyu Zhao Committed by Nick Thomas

Disable rugged auto-detect when Puma threads>1

If Puma is in use and Puma threads is more than 1, it is expected to
have performance lose by using Rugged patches, because of Rugged calls
does not release Ruby GVL while doing IO. So in this case, we will
ignore rugged automatic detect `can_use_disk?`.
parent 99c61a33
...@@ -8,9 +8,17 @@ module Gitlab ...@@ -8,9 +8,17 @@ module Gitlab
feature = Feature.get(feature_key) feature = Feature.get(feature_key)
return feature.enabled? if Feature.persisted?(feature) return feature.enabled? if Feature.persisted?(feature)
# Disable Rugged auto-detect(can_use_disk?) when Puma threads>1
# https://gitlab.com/gitlab-org/gitlab/issues/119326
return false if running_puma_with_multiple_threads?
Gitlab::GitalyClient.can_use_disk?(repo.storage) Gitlab::GitalyClient.can_use_disk?(repo.storage)
end end
def running_puma_with_multiple_threads?
Gitlab::Runtime.puma? && ::Puma.cli_config.options[:max_threads] > 1
end
def execute_rugged_call(method_name, *args) def execute_rugged_call(method_name, *args)
Gitlab::GitalyClient::StorageSettings.allow_disk_access do Gitlab::GitalyClient::StorageSettings.allow_disk_access do
start = Gitlab::Metrics::System.monotonic_time start = Gitlab::Metrics::System.monotonic_time
......
...@@ -53,30 +53,46 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do ...@@ -53,30 +53,46 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
allow(Feature).to receive(:persisted?).with(feature_flag).and_return(false) allow(Feature).to receive(:persisted?).with(feature_flag).and_return(false)
end end
it 'returns true when gitaly matches disk' do context 'when running puma with multiple threads' do
expect(subject.use_rugged?(repository, feature_flag_name)).to be true before do
allow(subject).to receive(:running_puma_with_multiple_threads?).and_return(true)
end
it 'returns false' do
expect(subject.use_rugged?(repository, feature_flag_name)).to be false
end
end end
it 'returns false when disk access fails' do context 'when not running puma with multiple threads' do
allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist") before do
allow(subject).to receive(:running_puma_with_multiple_threads?).and_return(false)
end
expect(subject.use_rugged?(repository, feature_flag_name)).to be false it 'returns true when gitaly matches disk' do
end expect(subject.use_rugged?(repository, feature_flag_name)).to be true
end
it "returns false when gitaly doesn't match disk" do it 'returns false when disk access fails' do
allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file) allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist")
expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey expect(subject.use_rugged?(repository, feature_flag_name)).to be false
end
File.delete(temp_gitaly_metadata_file) it "returns false when gitaly doesn't match disk" do
end allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file)
it "doesn't lead to a second rpc call because gitaly client should use the cached value" do expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
expect(subject.use_rugged?(repository, feature_flag_name)).to be true
expect(Gitlab::GitalyClient).not_to receive(:filesystem_id) File.delete(temp_gitaly_metadata_file)
end
subject.use_rugged?(repository, feature_flag_name) it "doesn't lead to a second rpc call because gitaly client should use the cached value" do
expect(subject.use_rugged?(repository, feature_flag_name)).to be true
expect(Gitlab::GitalyClient).not_to receive(:filesystem_id)
subject.use_rugged?(repository, feature_flag_name)
end
end end
end end
...@@ -99,6 +115,37 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do ...@@ -99,6 +115,37 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
end end
end end
describe '#running_puma_with_multiple_threads?' do
context 'when using Puma' do
before do
stub_const('::Puma', class_double('Puma'))
allow(Gitlab::Runtime).to receive(:puma?).and_return(true)
end
it 'returns false for single thread Puma' do
allow(::Puma).to receive_message_chain(:cli_config, :options).and_return(max_threads: 1)
expect(subject.running_puma_with_multiple_threads?).to be false
end
it 'returns true for multi-threaded Puma' do
allow(::Puma).to receive_message_chain(:cli_config, :options).and_return(max_threads: 2)
expect(subject.running_puma_with_multiple_threads?).to be true
end
end
context 'when not using Puma' do
before do
allow(Gitlab::Runtime).to receive(:puma?).and_return(false)
end
it 'returns false' do
expect(subject.running_puma_with_multiple_threads?).to be false
end
end
end
def create_temporary_gitaly_metadata_file def create_temporary_gitaly_metadata_file
tmp = Tempfile.new('.gitaly-metadata') tmp = Tempfile.new('.gitaly-metadata')
gitaly_metadata = { gitaly_metadata = {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment