Commit 591ee4e3 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Perform the stat check multiple times when checking a storage

Instead of only checking once within a timeout, check multiple times
within a timeout.

That means with a timeout of 30 seconds and 3 retries. Each try would
be allowed 20 seconds.
parent 430e7671
...@@ -91,7 +91,7 @@ module Gitlab ...@@ -91,7 +91,7 @@ module Gitlab
return @storage_available if @storage_available return @storage_available if @storage_available
if @storage_available = Gitlab::Git::Storage::ForkedStorageCheck if @storage_available = Gitlab::Git::Storage::ForkedStorageCheck
.storage_available?(storage_path, storage_timeout) .storage_available?(storage_path, storage_timeout, access_retries)
track_storage_accessible track_storage_accessible
else else
track_storage_inaccessible track_storage_inaccessible
......
...@@ -4,8 +4,17 @@ module Gitlab ...@@ -4,8 +4,17 @@ module Gitlab
module ForkedStorageCheck module ForkedStorageCheck
extend self extend self
def storage_available?(path, timeout_seconds = 5) def storage_available?(path, timeout_seconds = 5, retries = 1)
status = timeout_check(path, timeout_seconds) partial_timeout = timeout_seconds / retries
status = timeout_check(path, partial_timeout)
# If the status check did not succeed the first time, we retry a few
# more times to avoid one-off failures
current_attempts = 1
while current_attempts < retries && !status.success?
status = timeout_check(path, partial_timeout)
current_attempts += 1
end
status.success? status.success?
end end
......
...@@ -181,13 +181,24 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: ...@@ -181,13 +181,24 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
expect(circuit_breaker.last_failure).to be_nil expect(circuit_breaker.last_failure).to be_nil
end end
it 'only accessibility check once' do it 'only performs the accessibility check once' do
expect(Gitlab::Git::Storage::ForkedStorageCheck) expect(Gitlab::Git::Storage::ForkedStorageCheck)
.to receive(:storage_available?).once.and_call_original .to receive(:storage_available?).once.and_call_original
2.times { circuit_breaker.perform { '' } } 2.times { circuit_breaker.perform { '' } }
end end
it 'calls the check with the correct arguments' do
stub_application_setting(circuitbreaker_storage_timeout: 30,
circuitbreaker_access_retries: 3)
expect(Gitlab::Git::Storage::ForkedStorageCheck)
.to receive(:storage_available?).with(TestEnv.repos_path, 30, 3)
.and_call_original
circuit_breaker.perform { '' }
end
context 'with the feature disabled' do context 'with the feature disabled' do
it 'returns the block without checking accessibility' do it 'returns the block without checking accessibility' do
stub_feature_flags(git_storage_circuit_breaker: false) stub_feature_flags(git_storage_circuit_breaker: false)
......
...@@ -33,6 +33,21 @@ describe Gitlab::Git::Storage::ForkedStorageCheck, broken_storage: true, skip_da ...@@ -33,6 +33,21 @@ describe Gitlab::Git::Storage::ForkedStorageCheck, broken_storage: true, skip_da
expect(runtime).to be < 1.0 expect(runtime).to be < 1.0
end end
it 'will try the specified amount of times before failing' do
allow(described_class).to receive(:check_filesystem_in_process) do
Process.spawn("sleep 10")
end
expect(Process).to receive(:spawn).with('sleep 10').twice
.and_call_original
runtime = Benchmark.realtime do
described_class.storage_available?(existing_path, 0.5, 2)
end
expect(runtime).to be < 1.0
end
describe 'when using paths with spaces' do describe 'when using paths with spaces' do
let(:test_dir) { Rails.root.join('tmp', 'tests', 'storage_check') } let(:test_dir) { Rails.root.join('tmp', 'tests', 'storage_check') }
let(:path_with_spaces) { File.join(test_dir, 'path with spaces') } let(:path_with_spaces) { File.join(test_dir, 'path with spaces') }
......
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