diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2017-10-12 15:13:59 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2017-10-17 12:50:32 +0300 |
commit | 619021fd7a6ac68e2821f89f746fea3a72c25e3b (patch) | |
tree | 326b20849aec1b2de3ad5fafcd902291504b627d | |
parent | 85640c5cbbf3e08f562ecbebfeabc64862e85bed (diff) |
Read circuitbreaker settings from `Gitlab::CurrentSettings`
Instead of from the configuration file
-rw-r--r-- | config/gitlab.yml.example | 8 | ||||
-rw-r--r-- | config/initializers/1_settings.rb | 11 | ||||
-rw-r--r-- | doc/administration/repository_storage_paths.md | 27 | ||||
-rw-r--r-- | lib/gitlab/git/storage/circuit_breaker.rb | 12 | ||||
-rw-r--r-- | lib/gitlab/git/storage/circuit_breaker_settings.rb | 29 | ||||
-rw-r--r-- | lib/gitlab/git/storage/null_circuit_breaker.rb | 13 | ||||
-rw-r--r-- | spec/features/admin/admin_health_check_spec.rb | 4 | ||||
-rw-r--r-- | spec/initializers/settings_spec.rb | 20 | ||||
-rw-r--r-- | spec/lib/gitlab/git/storage/circuit_breaker_spec.rb | 54 | ||||
-rw-r--r-- | spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb | 4 | ||||
-rw-r--r-- | spec/support/stub_configuration.rb | 4 |
11 files changed, 80 insertions, 106 deletions
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 1069c7be5f0..4bfa5be0136 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -522,11 +522,6 @@ production: &base path: /home/git/repositories/ gitaly_address: unix:/home/git/gitlab/tmp/sockets/private/gitaly.socket # TCP connections are supported too (e.g. tcp://host:port) # gitaly_token: 'special token' # Optional: override global gitaly.token for this storage. - failure_count_threshold: 10 # number of failures before stopping attempts - failure_wait_time: 30 # Seconds after an access failure before allowing access again - failure_reset_time: 1800 # Time in seconds to expire failures - storage_timeout: 30 # Time in seconds to wait before aborting a storage access attempt - ## Backup settings backup: @@ -659,9 +654,6 @@ 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/1_settings.rb b/config/initializers/1_settings.rb index a4b7c1a3919..d8d48e3ae6b 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -453,17 +453,6 @@ Settings.repositories.storages.each do |key, storage| # Expand relative paths storage['path'] = Settings.absolute(storage['path']) - # Set failure defaults - storage['failure_count_threshold'] ||= 10 - storage['failure_wait_time'] ||= 30 - storage['failure_reset_time'] ||= 1800 - storage['storage_timeout'] ||= 5 - # Set turn strings into numbers - storage['failure_count_threshold'] = storage['failure_count_threshold'].to_i - storage['failure_wait_time'] = storage['failure_wait_time'].to_i - storage['failure_reset_time'] = storage['failure_reset_time'].to_i - # We might want to have a timeout shorter than 1 second. - storage['storage_timeout'] = storage['storage_timeout'].to_f Settings.repositories.storages[key] = storage end diff --git a/doc/administration/repository_storage_paths.md b/doc/administration/repository_storage_paths.md index 624a908b3a3..86ce87b130d 100644 --- a/doc/administration/repository_storage_paths.md +++ b/doc/administration/repository_storage_paths.md @@ -114,11 +114,7 @@ The configuration could look as follows: ```ruby git_data_dirs({ "default" => { - "path" => "/mnt/nfs-01/git-data", - "failure_count_threshold" => 10, - "failure_wait_time" => 30, - "failure_reset_time" => 1800, - "storage_timeout" => 5 + "path" => "/mnt/nfs-01/git-data" } }) ``` @@ -136,31 +132,10 @@ The configuration could look as follows: storages: # You must have at least a `default` storage path. default: path: /home/git/repositories/ - failure_count_threshold: 10 # number of failures before stopping attempts - failure_wait_time: 30 # Seconds after last access failure before trying again - failure_reset_time: 1800 # Time in seconds to expire failures - storage_timeout: 5 # Time in seconds to wait before aborting a storage access attempt ``` 1. Save the file and [restart GitLab][restart-gitlab] for the changes to take effect. - -**`failure_count_threshold`:** The number of failures of after which GitLab will -completely prevent access to the storage. The number of failures can be reset in -the admin interface: `https://gitlab.example.com/admin/health_check` or using the -[api](../api/repository_storage_health.md) to allow access to the storage again. - -**`failure_wait_time`:** When access to a storage fails. GitLab will prevent -access to the storage for the time specified here. This allows the filesystem to -recover without. - -**`failure_reset_time`:** The time in seconds GitLab will keep failure -information. When no failures occur during this time, information about the -mount is reset. - -**`storage_timeout`:** The time in seconds GitLab will try to access storage. -After this time a timeout error will be raised. - When storage failures occur, this will be visible in the admin interface like this: ![failing storage](img/failing_storage.png) diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb index 1eaa2d83fb6..4e2d261f461 100644 --- a/lib/gitlab/git/storage/circuit_breaker.rb +++ b/lib/gitlab/git/storage/circuit_breaker.rb @@ -2,15 +2,13 @@ module Gitlab module Git module Storage class CircuitBreaker + include CircuitBreakerSettings + FailureInfo = Struct.new(:last_failure, :failure_count) attr_reader :storage, :hostname, - :storage_path, - :failure_count_threshold, - :failure_wait_time, - :failure_reset_time, - :storage_timeout + :storage_path delegate :last_failure, :failure_count, to: :failure_info @@ -53,10 +51,6 @@ module Gitlab config = Gitlab.config.repositories.storages[@storage] @storage_path = config['path'] - @failure_count_threshold = config['failure_count_threshold'] - @failure_wait_time = config['failure_wait_time'] - @failure_reset_time = config['failure_reset_time'] - @storage_timeout = config['storage_timeout'] end def perform diff --git a/lib/gitlab/git/storage/circuit_breaker_settings.rb b/lib/gitlab/git/storage/circuit_breaker_settings.rb new file mode 100644 index 00000000000..d2313fe7c1b --- /dev/null +++ b/lib/gitlab/git/storage/circuit_breaker_settings.rb @@ -0,0 +1,29 @@ +module Gitlab + module Git + module Storage + module CircuitBreakerSettings + def failure_count_threshold + application_settings.circuitbreaker_failure_count_threshold + end + + def failure_wait_time + application_settings.circuitbreaker_failure_wait_time + end + + def failure_reset_time + application_settings.circuitbreaker_failure_reset_time + end + + def storage_timeout + application_settings.circuitbreaker_storage_timeout + end + + private + + def application_settings + Gitlab::CurrentSettings.current_application_settings + end + end + end + end +end diff --git a/lib/gitlab/git/storage/null_circuit_breaker.rb b/lib/gitlab/git/storage/null_circuit_breaker.rb index 297c043d054..60c6791a7e4 100644 --- a/lib/gitlab/git/storage/null_circuit_breaker.rb +++ b/lib/gitlab/git/storage/null_circuit_breaker.rb @@ -2,15 +2,14 @@ module Gitlab module Git module Storage class NullCircuitBreaker + include CircuitBreakerSettings + # These will have actual values attr_reader :storage, :hostname # These will always have nil values - attr_reader :storage_path, - :failure_wait_time, - :failure_reset_time, - :storage_timeout + attr_reader :storage_path def initialize(storage, hostname, error: nil) @storage = storage @@ -26,16 +25,12 @@ module Gitlab !!@error end - def failure_count_threshold - 1 - end - def last_failure circuit_broken? ? Time.now : nil end def failure_count - circuit_broken? ? 1 : 0 + circuit_broken? ? failure_count_threshold : 0 end def failure_info diff --git a/spec/features/admin/admin_health_check_spec.rb b/spec/features/admin/admin_health_check_spec.rb index 09e6965849a..4430fc15501 100644 --- a/spec/features/admin/admin_health_check_spec.rb +++ b/spec/features/admin/admin_health_check_spec.rb @@ -65,9 +65,11 @@ feature "Admin Health Check", :feature, :broken_storage do it 'shows storage failure information' do hostname = Gitlab::Environment.hostname + maximum_failures = Gitlab::CurrentSettings.current_application_settings + .circuitbreaker_failure_count_threshold expect(page).to have_content('broken: failed storage access attempt on host:') - expect(page).to have_content("#{hostname}: 1 of 10 failures.") + expect(page).to have_content("#{hostname}: 1 of #{maximum_failures} failures.") end it 'allows resetting storage failures' do diff --git a/spec/initializers/settings_spec.rb b/spec/initializers/settings_spec.rb index 9a974e70e8c..a11824d0ac5 100644 --- a/spec/initializers/settings_spec.rb +++ b/spec/initializers/settings_spec.rb @@ -18,26 +18,6 @@ describe Settings do end end - describe '#repositories' do - it 'assigns the default failure attributes' do - repository_settings = Gitlab.config.repositories.storages['broken'] - - expect(repository_settings['failure_count_threshold']).to eq(10) - expect(repository_settings['failure_wait_time']).to eq(30) - expect(repository_settings['failure_reset_time']).to eq(1800) - expect(repository_settings['storage_timeout']).to eq(5) - end - - it 'can be accessed with dot syntax all the way down' do - expect(Gitlab.config.repositories.storages.broken.failure_count_threshold).to eq(10) - end - - it 'can be accessed in a very specific way that breaks without reassigning each element with Settingslogic' do - storage_settings = Gitlab.config.repositories.storages['broken'] - expect(storage_settings.failure_count_threshold).to eq(10) - end - end - describe '#host_without_www' do context 'URL with protocol' do it 'returns the host' do diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb index 98cf7966dad..68eb93eb9f9 100644 --- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb +++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb @@ -10,18 +10,10 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: # 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 + 'path' => TestEnv.repos_path }, 'broken' => { - 'path' => 'tmp/tests/non-existent-repositories', - 'failure_count_threshold' => 10, - 'failure_wait_time' => 30, - 'failure_reset_time' => 1800, - 'storage_timeout' => 5 + 'path' => 'tmp/tests/non-existent-repositories' }, 'nopath' => { 'path' => nil } ) @@ -75,10 +67,39 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: expect(circuit_breaker.hostname).to eq(hostname) expect(circuit_breaker.storage).to eq('default') expect(circuit_breaker.storage_path).to eq(TestEnv.repos_path) - expect(circuit_breaker.failure_count_threshold).to eq(10) - expect(circuit_breaker.failure_wait_time).to eq(30) - expect(circuit_breaker.failure_reset_time).to eq(1800) - expect(circuit_breaker.storage_timeout).to eq(5) + end + end + + context 'circuitbreaker settings' do + before do + stub_application_setting(circuitbreaker_failure_count_threshold: 0, + circuitbreaker_failure_wait_time: 1, + circuitbreaker_failure_reset_time: 2, + circuitbreaker_storage_timeout: 3) + end + + describe '#failure_count_threshold' do + it 'reads the value from settings' do + expect(circuit_breaker.failure_count_threshold).to eq(0) + end + end + + describe '#failure_wait_time' do + it 'reads the value from settings' do + expect(circuit_breaker.failure_wait_time).to eq(1) + end + end + + describe '#failure_reset_time' do + it 'reads the value from settings' do + expect(circuit_breaker.failure_reset_time).to eq(2) + end + end + + describe '#storage_timeout' do + it 'reads the value from settings' do + expect(circuit_breaker.storage_timeout).to eq(3) + end end end @@ -151,10 +172,7 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: context 'the `failure_wait_time` is set to 0' do before do - stub_storage_settings('default' => { - 'failure_wait_time' => 0, - 'path' => TestEnv.repos_path - }) + stub_application_setting(circuitbreaker_failure_wait_time: 0) end it 'is working even when there is a recent failure' do diff --git a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb index 0e645008c88..7ee6d2f3709 100644 --- a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb +++ b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb @@ -54,6 +54,10 @@ describe Gitlab::Git::Storage::NullCircuitBreaker do end describe '#failure_count_threshold' do + before do + stub_application_setting(circuitbreaker_failure_count_threshold: 1) + end + it { expect(breaker.failure_count_threshold).to eq(1) } end diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb index 2dfb4d4a07f..4d448a55978 100644 --- a/spec/support/stub_configuration.rb +++ b/spec/support/stub_configuration.rb @@ -43,10 +43,6 @@ module StubConfiguration messages['default'] ||= Gitlab.config.repositories.storages.default messages.each do |storage_name, storage_settings| storage_settings['path'] = TestEnv.repos_path unless storage_settings.key?('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(Settingslogic.new(messages)) |