Commit 619021fd authored by Bob Van Landuyt's avatar Bob Van Landuyt

Read circuitbreaker settings from `Gitlab::CurrentSettings`

Instead of from the configuration file
parent 85640c5c
...@@ -522,11 +522,6 @@ production: &base ...@@ -522,11 +522,6 @@ production: &base
path: /home/git/repositories/ 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_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. # 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 settings
backup: backup:
...@@ -659,9 +654,6 @@ test: ...@@ -659,9 +654,6 @@ test:
default: default:
path: tmp/tests/repositories/ path: tmp/tests/repositories/
gitaly_address: unix:tmp/tests/gitaly/gitaly.socket gitaly_address: unix:tmp/tests/gitaly/gitaly.socket
failure_count_threshold: 999999
failure_wait_time: 0
storage_timeout: 30
broken: broken:
path: tmp/tests/non-existent-repositories path: tmp/tests/non-existent-repositories
gitaly_address: unix:tmp/tests/gitaly/gitaly.socket gitaly_address: unix:tmp/tests/gitaly/gitaly.socket
......
...@@ -453,17 +453,6 @@ Settings.repositories.storages.each do |key, storage| ...@@ -453,17 +453,6 @@ Settings.repositories.storages.each do |key, storage|
# Expand relative paths # Expand relative paths
storage['path'] = Settings.absolute(storage['path']) 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 Settings.repositories.storages[key] = storage
end end
......
...@@ -114,11 +114,7 @@ The configuration could look as follows: ...@@ -114,11 +114,7 @@ The configuration could look as follows:
```ruby ```ruby
git_data_dirs({ git_data_dirs({
"default" => { "default" => {
"path" => "/mnt/nfs-01/git-data", "path" => "/mnt/nfs-01/git-data"
"failure_count_threshold" => 10,
"failure_wait_time" => 30,
"failure_reset_time" => 1800,
"storage_timeout" => 5
} }
}) })
``` ```
...@@ -136,31 +132,10 @@ The configuration could look as follows: ...@@ -136,31 +132,10 @@ The configuration could look as follows:
storages: # You must have at least a `default` storage path. storages: # You must have at least a `default` storage path.
default: default:
path: /home/git/repositories/ 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. 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: When storage failures occur, this will be visible in the admin interface like this:
![failing storage](img/failing_storage.png) ![failing storage](img/failing_storage.png)
......
...@@ -2,15 +2,13 @@ module Gitlab ...@@ -2,15 +2,13 @@ module Gitlab
module Git module Git
module Storage module Storage
class CircuitBreaker class CircuitBreaker
include CircuitBreakerSettings
FailureInfo = Struct.new(:last_failure, :failure_count) FailureInfo = Struct.new(:last_failure, :failure_count)
attr_reader :storage, attr_reader :storage,
:hostname, :hostname,
:storage_path, :storage_path
:failure_count_threshold,
:failure_wait_time,
:failure_reset_time,
:storage_timeout
delegate :last_failure, :failure_count, to: :failure_info delegate :last_failure, :failure_count, to: :failure_info
...@@ -53,10 +51,6 @@ module Gitlab ...@@ -53,10 +51,6 @@ module Gitlab
config = Gitlab.config.repositories.storages[@storage] config = Gitlab.config.repositories.storages[@storage]
@storage_path = config['path'] @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 end
def perform def perform
......
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
...@@ -2,15 +2,14 @@ module Gitlab ...@@ -2,15 +2,14 @@ module Gitlab
module Git module Git
module Storage module Storage
class NullCircuitBreaker class NullCircuitBreaker
include CircuitBreakerSettings
# These will have actual values # These will have actual values
attr_reader :storage, attr_reader :storage,
:hostname :hostname
# These will always have nil values # These will always have nil values
attr_reader :storage_path, attr_reader :storage_path
:failure_wait_time,
:failure_reset_time,
:storage_timeout
def initialize(storage, hostname, error: nil) def initialize(storage, hostname, error: nil)
@storage = storage @storage = storage
...@@ -26,16 +25,12 @@ module Gitlab ...@@ -26,16 +25,12 @@ module Gitlab
!!@error !!@error
end end
def failure_count_threshold
1
end
def last_failure def last_failure
circuit_broken? ? Time.now : nil circuit_broken? ? Time.now : nil
end end
def failure_count def failure_count
circuit_broken? ? 1 : 0 circuit_broken? ? failure_count_threshold : 0
end end
def failure_info def failure_info
......
...@@ -65,9 +65,11 @@ feature "Admin Health Check", :feature, :broken_storage do ...@@ -65,9 +65,11 @@ feature "Admin Health Check", :feature, :broken_storage do
it 'shows storage failure information' do it 'shows storage failure information' do
hostname = Gitlab::Environment.hostname 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('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 end
it 'allows resetting storage failures' do it 'allows resetting storage failures' do
......
...@@ -18,26 +18,6 @@ describe Settings do ...@@ -18,26 +18,6 @@ describe Settings do
end end
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 describe '#host_without_www' do
context 'URL with protocol' do context 'URL with protocol' do
it 'returns the host' do it 'returns the host' do
......
...@@ -10,18 +10,10 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: ...@@ -10,18 +10,10 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
# Override test-settings for the circuitbreaker with something more realistic # Override test-settings for the circuitbreaker with something more realistic
# for these specs. # for these specs.
stub_storage_settings('default' => { stub_storage_settings('default' => {
'path' => TestEnv.repos_path, 'path' => TestEnv.repos_path
'failure_count_threshold' => 10,
'failure_wait_time' => 30,
'failure_reset_time' => 1800,
'storage_timeout' => 5
}, },
'broken' => { 'broken' => {
'path' => 'tmp/tests/non-existent-repositories', 'path' => 'tmp/tests/non-existent-repositories'
'failure_count_threshold' => 10,
'failure_wait_time' => 30,
'failure_reset_time' => 1800,
'storage_timeout' => 5
}, },
'nopath' => { 'path' => nil } 'nopath' => { 'path' => nil }
) )
...@@ -75,10 +67,39 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: ...@@ -75,10 +67,39 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
expect(circuit_breaker.hostname).to eq(hostname) expect(circuit_breaker.hostname).to eq(hostname)
expect(circuit_breaker.storage).to eq('default') expect(circuit_breaker.storage).to eq('default')
expect(circuit_breaker.storage_path).to eq(TestEnv.repos_path) expect(circuit_breaker.storage_path).to eq(TestEnv.repos_path)
expect(circuit_breaker.failure_count_threshold).to eq(10) end
expect(circuit_breaker.failure_wait_time).to eq(30) end
expect(circuit_breaker.failure_reset_time).to eq(1800)
expect(circuit_breaker.storage_timeout).to eq(5) 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
end end
...@@ -151,10 +172,7 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: ...@@ -151,10 +172,7 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
context 'the `failure_wait_time` is set to 0' do context 'the `failure_wait_time` is set to 0' do
before do before do
stub_storage_settings('default' => { stub_application_setting(circuitbreaker_failure_wait_time: 0)
'failure_wait_time' => 0,
'path' => TestEnv.repos_path
})
end end
it 'is working even when there is a recent failure' do it 'is working even when there is a recent failure' do
......
...@@ -54,6 +54,10 @@ describe Gitlab::Git::Storage::NullCircuitBreaker do ...@@ -54,6 +54,10 @@ describe Gitlab::Git::Storage::NullCircuitBreaker do
end end
describe '#failure_count_threshold' do 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) } it { expect(breaker.failure_count_threshold).to eq(1) }
end end
......
...@@ -43,10 +43,6 @@ module StubConfiguration ...@@ -43,10 +43,6 @@ module StubConfiguration
messages['default'] ||= Gitlab.config.repositories.storages.default messages['default'] ||= Gitlab.config.repositories.storages.default
messages.each do |storage_name, storage_settings| messages.each do |storage_name, storage_settings|
storage_settings['path'] = TestEnv.repos_path unless storage_settings.key?('path') 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 end
allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages)) allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages))
......
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