diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2017-10-19 09:05:19 +0200 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2017-10-23 13:53:49 +0300 |
commit | 591ee4e3611dff5f6feac3a73974da4e8d5cc69f (patch) | |
tree | 1bfba3dd17ef7df7277dd09ab4c37000fd671a22 | |
parent | 430e7671397a1c022b88da31328a5a81409671b5 (diff) | |
download | gitlab-ce-591ee4e3611dff5f6feac3a73974da4e8d5cc69f.tar.gz |
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.
-rw-r--r-- | lib/gitlab/git/storage/circuit_breaker.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/git/storage/forked_storage_check.rb | 13 | ||||
-rw-r--r-- | spec/lib/gitlab/git/storage/circuit_breaker_spec.rb | 13 | ||||
-rw-r--r-- | spec/lib/gitlab/git/storage/forked_storage_check_spec.rb | 15 |
4 files changed, 39 insertions, 4 deletions
diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb index 2ce97ff41f9..e53171fa3c5 100644 --- a/lib/gitlab/git/storage/circuit_breaker.rb +++ b/lib/gitlab/git/storage/circuit_breaker.rb @@ -91,7 +91,7 @@ module Gitlab return @storage_available if @storage_available if @storage_available = Gitlab::Git::Storage::ForkedStorageCheck - .storage_available?(storage_path, storage_timeout) + .storage_available?(storage_path, storage_timeout, access_retries) track_storage_accessible else track_storage_inaccessible diff --git a/lib/gitlab/git/storage/forked_storage_check.rb b/lib/gitlab/git/storage/forked_storage_check.rb index 91d8241f17b..1307f400700 100644 --- a/lib/gitlab/git/storage/forked_storage_check.rb +++ b/lib/gitlab/git/storage/forked_storage_check.rb @@ -4,8 +4,17 @@ module Gitlab module ForkedStorageCheck extend self - def storage_available?(path, timeout_seconds = 5) - status = timeout_check(path, timeout_seconds) + def storage_available?(path, timeout_seconds = 5, retries = 1) + 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? end diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb index e3f221aa863..c723ab18e67 100644 --- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb +++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb @@ -181,13 +181,24 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: expect(circuit_breaker.last_failure).to be_nil end - it 'only accessibility check once' do + it 'only performs the accessibility check once' do expect(Gitlab::Git::Storage::ForkedStorageCheck) .to receive(:storage_available?).once.and_call_original 2.times { circuit_breaker.perform { '' } } 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 it 'returns the block without checking accessibility' do stub_feature_flags(git_storage_circuit_breaker: false) diff --git a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb b/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb index c708b15853a..39a5d020bb4 100644 --- a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb +++ b/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb @@ -33,6 +33,21 @@ describe Gitlab::Git::Storage::ForkedStorageCheck, broken_storage: true, skip_da expect(runtime).to be < 1.0 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 let(:test_dir) { Rails.root.join('tmp', 'tests', 'storage_check') } let(:path_with_spaces) { File.join(test_dir, 'path with spaces') } |