diff options
author | Rémy Coutable <remy@rymai.me> | 2017-08-16 16:38:00 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-08-16 16:38:00 +0300 |
commit | 4e0ee10a721d73c7169a0dded4a38ba850459b70 (patch) | |
tree | b800119b35a955ade21c0d57d2bed0d7cad9e86e | |
parent | 853799923d000c0386e6895bb91ed05e2886e068 (diff) | |
parent | 93d56eb2a5763cb5f1ac89610bb2e1dc7f77a04a (diff) |
Merge branch 'bvl-circuitbreaker-settings-to-avoid-failures' into 'master'
Circuitbreaker settings to avoid spec-failures
Closes #36324
See merge request !13519
-rw-r--r-- | config/gitlab.yml.example | 3 | ||||
-rw-r--r-- | config/initializers/6_validations.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/git/storage/circuit_breaker_spec.rb | 59 | ||||
-rw-r--r-- | spec/support/stub_configuration.rb | 5 |
4 files changed, 58 insertions, 13 deletions
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index e73db08fcac..25285525846 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -649,6 +649,9 @@ test: default: path: tmp/tests/repositories/ gitaly_address: unix:tmp/tests/gitaly/gitaly.socket + failure_count_threshold: 999999 + failure_wait_time: 0 + storage_timeout: 30 broken: path: tmp/tests/non-existent-repositories gitaly_address: unix:tmp/tests/gitaly/gitaly.socket diff --git a/config/initializers/6_validations.rb b/config/initializers/6_validations.rb index 92ce4dd03cd..f8e67ce04c9 100644 --- a/config/initializers/6_validations.rb +++ b/config/initializers/6_validations.rb @@ -37,12 +37,12 @@ def validate_storages_config storage_validation_error("#{name} is not a valid storage, because it has no `path` key. Refer to gitlab.yml.example for an updated example") end - %w(failure_count_threshold failure_wait_time failure_reset_time storage_timeout).each do |setting| + %w(failure_count_threshold failure_reset_time storage_timeout).each do |setting| # Falling back to the defaults is fine! next if repository_storage[setting].nil? unless repository_storage[setting].to_f > 0 - storage_validation_error("#{setting}, for storage `#{name}` needs to be greater than 0") + storage_validation_error("`#{setting}` for storage `#{name}` needs to be greater than 0") end end end diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb index 9d1763b96ad..c86353abb7c 100644 --- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb +++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb @@ -1,9 +1,30 @@ require 'spec_helper' describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: true, broken_storage: true do - let(:circuit_breaker) { described_class.new('default') } + let(:storage_name) { 'default' } + let(:circuit_breaker) { described_class.new(storage_name) } let(:hostname) { Gitlab::Environment.hostname } - let(:cache_key) { "storage_accessible:default:#{hostname}" } + let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" } + + before do + # Override test-settings for the circuitbreaker with something more realistic + # for these specs. + stub_storage_settings('default' => { + 'path' => TestEnv.repos_path, + 'failure_count_threshold' => 10, + 'failure_wait_time' => 30, + 'failure_reset_time' => 1800, + 'storage_timeout' => 5 + }, + 'broken' => { + 'path' => 'tmp/tests/non-existent-repositories', + 'failure_count_threshold' => 10, + 'failure_wait_time' => 30, + 'failure_reset_time' => 1800, + 'storage_timeout' => 5 + } + ) + end def value_from_redis(name) Gitlab::Git::Storage.redis.with do |redis| @@ -96,14 +117,14 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: end describe '#circuit_broken?' do - it 'is closed when there is no last failure' do + it 'is working when there is no last failure' do set_in_redis(:last_failure, nil) set_in_redis(:failure_count, 0) expect(circuit_breaker.circuit_broken?).to be_falsey end - it 'is open when there was a recent failure' do + it 'is broken when there was a recent failure' do Timecop.freeze do set_in_redis(:last_failure, 1.second.ago.to_f) set_in_redis(:failure_count, 1) @@ -112,16 +133,34 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: end end - it 'is open when there are to many failures' do + it 'is broken when there are too many failures' do set_in_redis(:last_failure, 1.day.ago.to_f) set_in_redis(:failure_count, 200) expect(circuit_breaker.circuit_broken?).to be_truthy end + + context 'the `failure_wait_time` is set to 0' do + before do + stub_storage_settings('default' => { + 'failure_wait_time' => 0, + 'path' => TestEnv.repos_path + }) + end + + it 'is working even when there is a recent failure' do + Timecop.freeze do + set_in_redis(:last_failure, 0.seconds.ago.to_f) + set_in_redis(:failure_count, 1) + + expect(circuit_breaker.circuit_broken?).to be_falsey + end + end + end end describe "storage_available?" do - context 'when the storage is available' do + context 'the storage is available' do it 'tracks that the storage was accessible an raises the error' do expect(circuit_breaker).to receive(:track_storage_accessible) @@ -136,8 +175,8 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: end end - context 'when storage is not available' do - let(:circuit_breaker) { described_class.new('broken') } + context 'storage is not available' do + let(:storage_name) { 'broken' } it 'tracks that the storage was inaccessible' do expect(circuit_breaker).to receive(:track_storage_inaccessible) @@ -158,8 +197,8 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: end end - context 'when the storage is not available' do - let(:circuit_breaker) { described_class.new('broken') } + context 'the storage is not available' do + let(:storage_name) { 'broken' } it 'raises an error' do expect(circuit_breaker).to receive(:track_storage_inaccessible) diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb index 37c89d37aa0..45c10e78789 100644 --- a/spec/support/stub_configuration.rb +++ b/spec/support/stub_configuration.rb @@ -39,14 +39,17 @@ module StubConfiguration end def stub_storage_settings(messages) + # Default storage is always required + messages['default'] ||= Gitlab.config.repositories.storages.default messages.each do |storage_name, storage_settings| + storage_settings['path'] ||= TestEnv.repos_path storage_settings['failure_count_threshold'] ||= 10 storage_settings['failure_wait_time'] ||= 30 storage_settings['failure_reset_time'] ||= 1800 storage_settings['storage_timeout'] ||= 5 end - allow(Gitlab.config.repositories).to receive(:storages).and_return(messages) + allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages)) end private |