diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2018-10-09 07:59:42 +0200 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2018-10-10 09:08:18 +0200 |
commit | 30b4ce940d28804e0b38ea9ea4f89793d41392db (patch) | |
tree | ecbf29b27a726867d260521dc799214a4cd6d4c4 /spec | |
parent | 550f55745a3be5f86bafaf25b3bcc90beba8e2ac (diff) | |
download | gitlab-ce-30b4ce940d28804e0b38ea9ea4f89793d41392db.tar.gz |
Remove Git circuit breaker
Was introduced in the time that GitLab still used NFS, which is not
required anymore in most cases. By removing this, the API it calls will
return empty responses. This interface has to be removed in the next
major release, expected to be 12.0.
Diffstat (limited to 'spec')
22 files changed, 11 insertions, 936 deletions
diff --git a/spec/bin/storage_check_spec.rb b/spec/bin/storage_check_spec.rb deleted file mode 100644 index 02f6fcb6e3a..00000000000 --- a/spec/bin/storage_check_spec.rb +++ /dev/null @@ -1,13 +0,0 @@ -require 'spec_helper' - -describe 'bin/storage_check' do - it 'is executable' do - command = %w[bin/storage_check -t unix://the/path/to/a/unix-socket.sock -i 10 -d] - expected_output = 'Checking unix://the/path/to/a/unix-socket.sock every 10 seconds' - - output, status = Gitlab::Popen.popen(command, Rails.root.to_s) - - expect(status).to eq(0) - expect(output).to include(expected_output) - end -end diff --git a/spec/controllers/admin/health_check_controller_spec.rb b/spec/controllers/admin/health_check_controller_spec.rb index d15ee0021d9..e13401fc06b 100644 --- a/spec/controllers/admin/health_check_controller_spec.rb +++ b/spec/controllers/admin/health_check_controller_spec.rb @@ -8,18 +8,10 @@ describe Admin::HealthCheckController do end describe 'GET show' do - it 'loads the git storage health information' do + it 'loads the health information' do get :show - expect(assigns[:failing_storage_statuses]).not_to be_nil - end - end - - describe 'POST reset_storage_health' do - it 'resets all storage health information' do - expect(Gitlab::Git::Storage::FailureInfo).to receive(:reset_all!) - - post :reset_storage_health + expect(assigns[:errors]).not_to be_nil end end end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 2b28cfd16cc..a8556771edd 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -190,30 +190,6 @@ describe ApplicationController do end end - describe 'rescue from Gitlab::Git::Storage::Inaccessible' do - controller(described_class) do - def index - raise Gitlab::Git::Storage::Inaccessible.new('broken', 100) - end - end - - it 'renders a 503 when storage is not available' do - sign_in(create(:user)) - - get :index - - expect(response.status).to eq(503) - end - - it 'renders includes a Retry-After header' do - sign_in(create(:user)) - - get :index - - expect(response.headers['Retry-After']).to eq(100) - end - end - describe 'response format' do controller(described_class) do def index diff --git a/spec/controllers/health_controller_spec.rb b/spec/controllers/health_controller_spec.rb index d800ad7c187..ec73c89cb11 100644 --- a/spec/controllers/health_controller_spec.rb +++ b/spec/controllers/health_controller_spec.rb @@ -14,48 +14,6 @@ describe HealthController do stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') end - describe '#storage_check' do - before do - allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip) - end - - subject { post :storage_check } - - it 'checks all the configured storages' do - expect(Gitlab::Git::Storage::Checker).to receive(:check_all).and_call_original - - subject - end - - it 'returns the check interval' do - stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true') - stub_application_setting(circuitbreaker_check_interval: 10) - - subject - - expect(json_response['check_interval']).to eq(10) - end - - context 'with failing storages', :broken_storage do - before do - stub_storage_settings( - broken: { path: 'tmp/tests/non-existent-repositories' } - ) - end - - it 'includes the failure information' do - subject - - expected_results = [ - { 'storage' => 'broken', 'success' => false }, - { 'storage' => 'default', 'success' => true } - ] - - expect(json_response['results']).to eq(expected_results) - end - end - end - describe '#readiness' do shared_context 'endpoint responding with readiness data' do let(:request_params) { {} } diff --git a/spec/features/admin/admin_health_check_spec.rb b/spec/features/admin/admin_health_check_spec.rb index aaa3e8dc821..790051dd933 100644 --- a/spec/features/admin/admin_health_check_spec.rb +++ b/spec/features/admin/admin_health_check_spec.rb @@ -2,10 +2,11 @@ require 'spec_helper' describe "Admin Health Check", :feature do include StubENV + set(:admin) { create(:admin) } before do stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') - sign_in(create(:admin)) + sign_in(admin) end describe '#show' do @@ -56,27 +57,4 @@ describe "Admin Health Check", :feature do expect(page).to have_content('The server is on fire') end end - - context 'with repository storage failures', :broken_storage do - before do - visit admin_health_check_path - end - - it 'shows storage failure information' do - hostname = Gitlab::Environment.hostname - maximum_failures = Gitlab::CurrentSettings.current_application_settings - .circuitbreaker_failure_count_threshold - number_of_failures = maximum_failures + 1 - - expect(page).to have_content("broken: #{number_of_failures} failed storage access attempts:") - expect(page).to have_content("#{hostname}: #{number_of_failures} of #{maximum_failures} failures.") - end - - it 'allows resetting storage failures' do - click_button 'Reset git storage health information' - - expect(page).to have_content('Git storage health information has been reset') - expect(page).not_to have_content('failed storage access attempt') - end - end end diff --git a/spec/helpers/storage_health_helper_spec.rb b/spec/helpers/storage_health_helper_spec.rb deleted file mode 100644 index 874498e6338..00000000000 --- a/spec/helpers/storage_health_helper_spec.rb +++ /dev/null @@ -1,20 +0,0 @@ -require 'spec_helper' - -describe StorageHealthHelper do - describe '#failing_storage_health_message' do - let(:health) do - Gitlab::Git::Storage::Health.new( - "<script>alert('storage name');)</script>", - [] - ) - end - - it 'escapes storage names' do - escaped_storage_name = '<script>alert('storage name');)</script>' - - result = helper.failing_storage_health_message(health) - - expect(result).to include(escaped_storage_name) - end - end -end diff --git a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb index 18658588a40..4d5081b0a75 100644 --- a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb +++ b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb @@ -49,8 +49,6 @@ describe Gitlab::Cache::Ci::ProjectPipelineStatus, :clean_gitlab_redis_cache do end it 'only connects to redis twice' do - # Stub circuitbreaker so it doesn't count the redis connections in there - stub_circuit_breaker(project_without_status) expect(Gitlab::Redis::Cache).to receive(:with).exactly(2).and_call_original described_class.load_in_batch_for_projects([project_without_status]) @@ -302,13 +300,4 @@ describe Gitlab::Cache::Ci::ProjectPipelineStatus, :clean_gitlab_redis_cache do end end end - - def stub_circuit_breaker(project) - fake_circuitbreaker = double - allow(fake_circuitbreaker).to receive(:perform).and_yield - allow(project.repository.raw_repository) - .to receive(:circuit_breaker).and_return(fake_circuitbreaker) - allow(project.repository) - .to receive(:circuit_breaker).and_return(fake_circuitbreaker) - end end diff --git a/spec/lib/gitlab/git/storage/checker_spec.rb b/spec/lib/gitlab/git/storage/checker_spec.rb deleted file mode 100644 index d74c3bcb04c..00000000000 --- a/spec/lib/gitlab/git/storage/checker_spec.rb +++ /dev/null @@ -1,132 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::Storage::Checker, :clean_gitlab_redis_shared_state do - let(:storage_name) { 'default' } - let(:hostname) { Gitlab::Environment.hostname } - let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" } - - subject(:checker) { described_class.new(storage_name) } - - def value_from_redis(name) - Gitlab::Git::Storage.redis.with do |redis| - redis.hmget(cache_key, name) - end.first - end - - def set_in_redis(name, value) - Gitlab::Git::Storage.redis.with do |redis| - redis.hmset(cache_key, name, value) - end.first - end - - describe '.check_all' do - it 'calls a check for each storage' do - fake_checker_default = double - fake_checker_broken = double - fake_logger = fake_logger - - expect(described_class).to receive(:new).with('default', fake_logger) { fake_checker_default } - expect(described_class).to receive(:new).with('broken', fake_logger) { fake_checker_broken } - expect(fake_checker_default).to receive(:check_with_lease) - expect(fake_checker_broken).to receive(:check_with_lease) - - described_class.check_all(fake_logger) - end - - context 'with broken storage', :broken_storage do - it 'returns the results' do - expected_result = [ - { storage: 'default', success: true }, - { storage: 'broken', success: false } - ] - - expect(described_class.check_all).to eq(expected_result) - end - end - end - - describe '#initialize' do - it 'assigns the settings' do - expect(checker.hostname).to eq(hostname) - expect(checker.storage).to eq('default') - expect(checker.storage_path).to eq(TestEnv.repos_path) - end - end - - describe '#check_with_lease' do - it 'only allows one check at a time' do - expect(checker).to receive(:check).once { sleep 1 } - - thread = Thread.new { checker.check_with_lease } - checker.check_with_lease - thread.join - end - - it 'returns a result hash' do - expect(checker.check_with_lease).to eq(storage: 'default', success: true) - end - end - - describe '#check' do - it 'tracks that the storage was accessible' do - set_in_redis(:failure_count, 10) - set_in_redis(:last_failure, Time.now.to_f) - - checker.check - - expect(value_from_redis(:failure_count).to_i).to eq(0) - expect(value_from_redis(:last_failure)).to be_empty - expect(value_from_redis(:first_failure)).to be_empty - 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 - - checker.check - end - - it 'returns `true`' do - expect(checker.check).to eq(true) - end - - it 'maintains known storage keys' do - Timecop.freeze do - # Insert an old key to expire - old_entry = Time.now.to_i - 3.days.to_i - Gitlab::Git::Storage.redis.with do |redis| - redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, old_entry, 'to_be_removed') - end - - checker.check - - known_keys = Gitlab::Git::Storage.redis.with do |redis| - redis.zrange(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, -1) - end - - expect(known_keys).to contain_exactly(cache_key) - end - end - - context 'the storage is not available', :broken_storage do - let(:storage_name) { 'broken' } - - it 'tracks that the storage was inaccessible' do - Timecop.freeze do - expect { checker.check }.to change { value_from_redis(:failure_count).to_i }.by(1) - - expect(value_from_redis(:last_failure)).not_to be_empty - expect(value_from_redis(:first_failure)).not_to be_empty - end - end - - it 'returns `false`' do - expect(checker.check).to eq(false) - end - 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 deleted file mode 100644 index 210b90bfba9..00000000000 --- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb +++ /dev/null @@ -1,187 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::Storage::CircuitBreaker, :broken_storage do - let(:storage_name) { 'default' } - let(:circuit_breaker) { described_class.new(storage_name, hostname) } - let(:hostname) { Gitlab::Environment.hostname } - let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" } - - def set_in_redis(name, value) - Gitlab::Git::Storage.redis.with do |redis| - redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key) - redis.hmset(cache_key, name, value) - end.first - end - - before do - # Override test-settings for the circuitbreaker with something more realistic - # for these specs. - stub_storage_settings('default' => { - 'path' => TestEnv.repos_path - }, - 'broken' => { - 'path' => 'tmp/tests/non-existent-repositories' - }, - 'nopath' => { 'path' => nil } - ) - end - - describe '.for_storage', :request_store do - it 'only builds a single circuitbreaker per storage' do - expect(described_class).to receive(:new).once.and_call_original - - breaker = described_class.for_storage('default') - - expect(breaker).to be_a(described_class) - expect(described_class.for_storage('default')).to eq(breaker) - end - - it 'returns a broken circuit breaker for an unknown storage' do - expect(described_class.for_storage('unknown').circuit_broken?).to be_truthy - end - - it 'returns a broken circuit breaker when the path is not set' do - expect(described_class.for_storage('nopath').circuit_broken?).to be_truthy - end - end - - describe '#initialize' do - it 'assigns the settings' do - expect(circuit_breaker.hostname).to eq(hostname) - expect(circuit_breaker.storage).to eq('default') - 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, - circuitbreaker_access_retries: 4, - circuitbreaker_backoff_threshold: 5) - 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 '#check_interval' do - it 'reads the value from settings' do - expect(circuit_breaker.check_interval).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 - - describe '#access_retries' do - it 'reads the value from settings' do - expect(circuit_breaker.access_retries).to eq(4) - end - end - end - - describe '#perform' do - it 'raises the correct exception when the circuit is open' do - set_in_redis(:last_failure, 1.day.ago.to_f) - set_in_redis(:failure_count, 999) - - expect { |b| circuit_breaker.perform(&b) } - .to raise_error do |exception| - expect(exception).to be_kind_of(Gitlab::Git::Storage::CircuitOpen) - expect(exception.retry_after).to eq(1800) - end - end - - it 'yields the block' do - expect { |b| circuit_breaker.perform(&b) } - .to yield_control - end - - it 'checks if the storage is available' do - expect(circuit_breaker).to receive(:check_storage_accessible!) - .and_call_original - - circuit_breaker.perform { 'hello world' } - end - - it 'returns the value of the block' do - result = circuit_breaker.perform { 'return value' } - - expect(result).to eq('return value') - end - - it 'raises possible errors' do - expect { circuit_breaker.perform { raise Rugged::OSError.new('Broken') } } - .to raise_error(Rugged::OSError) - end - - context 'with the feature disabled' do - before do - stub_feature_flags(git_storage_circuit_breaker: false) - end - - it 'returns the block without checking accessibility' do - expect(circuit_breaker).not_to receive(:check_storage_accessible!) - - result = circuit_breaker.perform { 'hello' } - - expect(result).to eq('hello') - end - - it 'allows enabling the feature using an ENV var' do - stub_env('GIT_STORAGE_CIRCUIT_BREAKER', 'true') - expect(circuit_breaker).to receive(:check_storage_accessible!) - - result = circuit_breaker.perform { 'hello' } - - expect(result).to eq('hello') - end - end - end - - describe '#circuit_broken?' 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 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 - end - - describe '#last_failure' do - it 'returns the last failure time' do - time = Time.parse("2017-05-26 17:52:30") - set_in_redis(:last_failure, time.to_i) - - expect(circuit_breaker.last_failure).to eq(time) - end - end - - describe '#failure_count' do - it 'returns the failure count' do - set_in_redis(:failure_count, 7) - - expect(circuit_breaker.failure_count).to eq(7) - end - end -end diff --git a/spec/lib/gitlab/git/storage/failure_info_spec.rb b/spec/lib/gitlab/git/storage/failure_info_spec.rb deleted file mode 100644 index bae88fdda86..00000000000 --- a/spec/lib/gitlab/git/storage/failure_info_spec.rb +++ /dev/null @@ -1,70 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::Storage::FailureInfo, :broken_storage do - let(:storage_name) { 'default' } - let(:hostname) { Gitlab::Environment.hostname } - let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" } - - def value_from_redis(name) - Gitlab::Git::Storage.redis.with do |redis| - redis.hmget(cache_key, name) - end.first - end - - def set_in_redis(name, value) - Gitlab::Git::Storage.redis.with do |redis| - redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key) - redis.hmset(cache_key, name, value) - end.first - end - - describe '.reset_all!' do - it 'clears all entries form redis' do - set_in_redis(:failure_count, 10) - - described_class.reset_all! - - key_exists = Gitlab::Git::Storage.redis.with { |redis| redis.exists(cache_key) } - - expect(key_exists).to be_falsey - end - - it 'does not break when there are no keys in redis' do - expect { described_class.reset_all! }.not_to raise_error - end - end - - describe '.load' do - it 'loads failure information for a storage on a host' do - first_failure = Time.parse("2017-11-14 17:52:30") - last_failure = Time.parse("2017-11-14 18:54:37") - failure_count = 11 - - set_in_redis(:first_failure, first_failure.to_i) - set_in_redis(:last_failure, last_failure.to_i) - set_in_redis(:failure_count, failure_count.to_i) - - info = described_class.load(cache_key) - - expect(info.first_failure).to eq(first_failure) - expect(info.last_failure).to eq(last_failure) - expect(info.failure_count).to eq(failure_count) - end - end - - describe '#no_failures?' do - it 'is true when there are no failures' do - info = described_class.new(nil, nil, 0) - - expect(info.no_failures?).to be_truthy - end - - it 'is false when there are failures' do - info = described_class.new(Time.parse("2017-11-14 17:52:30"), - Time.parse("2017-11-14 18:54:37"), - 20) - - expect(info.no_failures?).to be_falsy - end - end -end diff --git a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb b/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb deleted file mode 100644 index 39a5d020bb4..00000000000 --- a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb +++ /dev/null @@ -1,73 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::Storage::ForkedStorageCheck, broken_storage: true, skip_database_cleaner: true do - let(:existing_path) do - existing_path = TestEnv.repos_path - FileUtils.mkdir_p(existing_path) - existing_path - end - - describe '.storage_accessible?' do - it 'detects when a storage is not available' do - expect(described_class.storage_available?('/non/existant/path')).to be_falsey - end - - it 'detects when a storage is available' do - expect(described_class.storage_available?(existing_path)).to be_truthy - end - - it 'returns false when the check takes to long' do - # We're forking a process here that takes too long - # It will be killed it's parent process will be killed by it's parent - # and waited for inside `Gitlab::Git::Storage::ForkedStorageCheck.timeout_check` - allow(described_class).to receive(:check_filesystem_in_process) do - Process.spawn("sleep 10") - end - result = true - - runtime = Benchmark.realtime do - result = described_class.storage_available?(existing_path, 0.5) - end - - expect(result).to be_falsey - 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') } - - around do |example| - FileUtils.mkdir_p(path_with_spaces) - example.run - FileUtils.rm_r(test_dir) - end - - it 'works for paths with spaces' do - expect(described_class.storage_available?(path_with_spaces)).to be_truthy - end - - it 'works for a realpath with spaces' do - symlink_location = File.join(test_dir, 'a symlink') - FileUtils.ln_s(path_with_spaces, symlink_location) - - expect(described_class.storage_available?(symlink_location)).to be_truthy - end - end - end -end diff --git a/spec/lib/gitlab/git/storage/health_spec.rb b/spec/lib/gitlab/git/storage/health_spec.rb deleted file mode 100644 index bb670fc5d94..00000000000 --- a/spec/lib/gitlab/git/storage/health_spec.rb +++ /dev/null @@ -1,58 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::Storage::Health, broken_storage: true do - let(:host1_key) { 'storage_accessible:broken:web01' } - let(:host2_key) { 'storage_accessible:default:kiq01' } - - def set_in_redis(cache_key, value) - Gitlab::Git::Storage.redis.with do |redis| - redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key) - redis.hmset(cache_key, :failure_count, value) - end.first - end - - describe '.for_failing_storages' do - it 'only includes health status for failures' do - set_in_redis(host1_key, 10) - set_in_redis(host2_key, 0) - - expect(described_class.for_failing_storages.map(&:storage_name)) - .to contain_exactly('broken') - end - end - - describe '.for_all_storages' do - it 'loads health status for all configured storages' do - healths = described_class.for_all_storages - - expect(healths.map(&:storage_name)).to contain_exactly('default', 'broken') - end - end - - describe '#failing_info' do - it 'only contains storages that have failures' do - health = described_class.new('broken', [{ name: host1_key, failure_count: 0 }, - { name: host2_key, failure_count: 3 }]) - - expect(health.failing_info).to contain_exactly({ name: host2_key, failure_count: 3 }) - end - end - - describe '#total_failures' do - it 'sums up all the failures' do - health = described_class.new('broken', [{ name: host1_key, failure_count: 2 }, - { name: host2_key, failure_count: 3 }]) - - expect(health.total_failures).to eq(5) - end - end - - describe '#failing_on_hosts' do - it 'collects only the failing hostnames' do - health = described_class.new('broken', [{ name: host1_key, failure_count: 2 }, - { name: host2_key, failure_count: 0 }]) - - expect(health.failing_on_hosts).to contain_exactly('web01') - end - end -end diff --git a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb deleted file mode 100644 index 93ad20011de..00000000000 --- a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb +++ /dev/null @@ -1,70 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git::Storage::NullCircuitBreaker do - let(:storage) { 'default' } - let(:hostname) { 'localhost' } - let(:error) { nil } - - subject(:breaker) { described_class.new(storage, hostname, error: error) } - - context 'with an error' do - let(:error) { Gitlab::Git::Storage::Misconfiguration.new('error') } - - describe '#perform' do - it { expect { breaker.perform { 'ok' } }.to raise_error(error) } - end - - describe '#circuit_broken?' do - it { expect(breaker.circuit_broken?).to be_truthy } - end - - describe '#last_failure' do - it { Timecop.freeze { expect(breaker.last_failure).to eq(Time.now) } } - end - - describe '#failure_count' do - it { expect(breaker.failure_count).to eq(breaker.failure_count_threshold) } - end - - describe '#failure_info' do - it { expect(breaker.failure_info.no_failures?).to be_falsy } - end - end - - context 'not broken' do - describe '#perform' do - it { expect(breaker.perform { 'ok' }).to eq('ok') } - end - - describe '#circuit_broken?' do - it { expect(breaker.circuit_broken?).to be_falsy } - end - - describe '#last_failure' do - it { expect(breaker.last_failure).to be_nil } - end - - describe '#failure_count' do - it { expect(breaker.failure_count).to eq(0) } - end - - describe '#failure_info' do - it { expect(breaker.failure_info.no_failures?).to be_truthy } - end - 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 - - it 'implements the CircuitBreaker interface' do - ours = described_class.public_instance_methods - theirs = Gitlab::Git::Storage::CircuitBreaker.public_instance_methods - - expect(theirs - ours).to be_empty - end -end diff --git a/spec/lib/gitlab/storage_check/cli_spec.rb b/spec/lib/gitlab/storage_check/cli_spec.rb deleted file mode 100644 index 6db0925899c..00000000000 --- a/spec/lib/gitlab/storage_check/cli_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'spec_helper' - -describe Gitlab::StorageCheck::CLI do - let(:options) { Gitlab::StorageCheck::Options.new('unix://tmp/socket.sock', nil, 1, false) } - subject(:runner) { described_class.new(options) } - - describe '#update_settings' do - it 'updates the interval when changed in a valid response and logs the change' do - fake_response = double - expect(fake_response).to receive(:valid?).and_return(true) - expect(fake_response).to receive(:check_interval).and_return(42) - expect(runner.logger).to receive(:info) - - runner.update_settings(fake_response) - - expect(options.interval).to eq(42) - end - end -end diff --git a/spec/lib/gitlab/storage_check/gitlab_caller_spec.rb b/spec/lib/gitlab/storage_check/gitlab_caller_spec.rb deleted file mode 100644 index d869022fd31..00000000000 --- a/spec/lib/gitlab/storage_check/gitlab_caller_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -require 'spec_helper' - -describe Gitlab::StorageCheck::GitlabCaller do - let(:options) { Gitlab::StorageCheck::Options.new('unix://tmp/socket.sock', nil, nil, false) } - subject(:gitlab_caller) { described_class.new(options) } - - describe '#call!' do - context 'when a socket is given' do - it 'calls a socket' do - fake_connection = double - expect(fake_connection).to receive(:post) - expect(Excon).to receive(:new).with('unix://tmp/socket.sock', socket: "tmp/socket.sock") { fake_connection } - - gitlab_caller.call! - end - end - - context 'when a host is given' do - let(:options) { Gitlab::StorageCheck::Options.new('http://localhost:8080', nil, nil, false) } - - it 'it calls a http response' do - fake_connection = double - expect(Excon).to receive(:new).with('http://localhost:8080', socket: nil) { fake_connection } - expect(fake_connection).to receive(:post) - - gitlab_caller.call! - end - end - end - - describe '#headers' do - it 'Adds the JSON header' do - headers = gitlab_caller.headers - - expect(headers['Content-Type']).to eq('application/json') - end - - context 'when a token was provided' do - let(:options) { Gitlab::StorageCheck::Options.new('unix://tmp/socket.sock', 'atoken', nil, false) } - - it 'adds it to the headers' do - expect(gitlab_caller.headers['TOKEN']).to eq('atoken') - end - end - end -end diff --git a/spec/lib/gitlab/storage_check/option_parser_spec.rb b/spec/lib/gitlab/storage_check/option_parser_spec.rb deleted file mode 100644 index cad4dfbefcf..00000000000 --- a/spec/lib/gitlab/storage_check/option_parser_spec.rb +++ /dev/null @@ -1,31 +0,0 @@ -require 'spec_helper' - -describe Gitlab::StorageCheck::OptionParser do - describe '.parse!' do - it 'assigns all options' do - args = %w(--target unix://tmp/hello/world.sock --token thetoken --interval 42) - - options = described_class.parse!(args) - - expect(options.token).to eq('thetoken') - expect(options.interval).to eq(42) - expect(options.target).to eq('unix://tmp/hello/world.sock') - end - - it 'requires the interval to be a number' do - args = %w(--target unix://tmp/hello/world.sock --interval fortytwo) - - expect { described_class.parse!(args) }.to raise_error(OptionParser::InvalidArgument) - end - - it 'raises an error if the scheme is not included' do - args = %w(--target tmp/hello/world.sock) - - expect { described_class.parse!(args) }.to raise_error(OptionParser::InvalidArgument) - end - - it 'raises an error if both socket and host are missing' do - expect { described_class.parse!([]) }.to raise_error(OptionParser::InvalidArgument) - end - end -end diff --git a/spec/lib/gitlab/storage_check/response_spec.rb b/spec/lib/gitlab/storage_check/response_spec.rb deleted file mode 100644 index 0ff2963e443..00000000000 --- a/spec/lib/gitlab/storage_check/response_spec.rb +++ /dev/null @@ -1,54 +0,0 @@ -require 'spec_helper' - -describe Gitlab::StorageCheck::Response do - let(:fake_json) do - { - check_interval: 42, - results: [ - { storage: 'working', success: true }, - { storage: 'skipped', success: nil }, - { storage: 'failing', success: false } - ] - }.to_json - end - - let(:fake_http_response) do - fake_response = instance_double("Excon::Response - Status check") - allow(fake_response).to receive(:status).and_return(200) - allow(fake_response).to receive(:body).and_return(fake_json) - allow(fake_response).to receive(:headers).and_return('Content-Type' => 'application/json') - - fake_response - end - let(:response) { described_class.new(fake_http_response) } - - describe '#valid?' do - it 'is valid for a success response with parseable JSON' do - expect(response).to be_valid - end - end - - describe '#check_interval' do - it 'returns the result from the JSON' do - expect(response.check_interval).to eq(42) - end - end - - describe '#responsive_shards' do - it 'contains the names of working shards' do - expect(response.responsive_shards).to contain_exactly('working') - end - end - - describe '#skipped_shards' do - it 'contains the names of skipped shards' do - expect(response.skipped_shards).to contain_exactly('skipped') - end - end - - describe '#failing_shards' do - it 'contains the name of failing shards' do - expect(response.failing_shards).to contain_exactly('failing') - end - end -end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index dcfc80daa57..87b91286168 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -141,19 +141,6 @@ describe ApplicationSetting do end end - context 'circuitbreaker settings' do - [:circuitbreaker_failure_count_threshold, - :circuitbreaker_check_interval, - :circuitbreaker_failure_reset_time, - :circuitbreaker_storage_timeout].each do |field| - it "Validates #{field} as number" do - is_expected.to validate_numericality_of(field) - .only_integer - .is_greater_than_or_equal_to(0) - end - end - end - context 'repository storages' do before do storages = { diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 77e549d9528..aed8e02cc23 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -30,7 +30,7 @@ describe Repository do def expect_to_raise_storage_error expect { yield }.to raise_error do |exception| - storage_exceptions = [Gitlab::Git::Storage::Inaccessible, Gitlab::Git::CommandError, GRPC::Unavailable] + storage_exceptions = [Gitlab::Git::CommandError, GRPC::Unavailable] known_exception = storage_exceptions.select { |e| exception.is_a?(e) } expect(known_exception).not_to be_nil diff --git a/spec/requests/api/circuit_breakers_spec.rb b/spec/requests/api/circuit_breakers_spec.rb index fe76f057115..6c7cb151c74 100644 --- a/spec/requests/api/circuit_breakers_spec.rb +++ b/spec/requests/api/circuit_breakers_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' describe API::CircuitBreakers do - let(:user) { create(:user) } - let(:admin) { create(:admin) } + set(:user) { create(:user) } + set(:admin) { create(:admin) } describe 'GET circuit_breakers/repository_storage' do it 'returns a 401 for anonymous users' do @@ -18,37 +18,26 @@ describe API::CircuitBreakers do end it 'returns an Array of storages' do - expect(Gitlab::Git::Storage::Health).to receive(:for_all_storages) do - [Gitlab::Git::Storage::Health.new('broken', [{ name: 'prefix:broken:web01', failure_count: 4 }])] - end - get api('/circuit_breakers/repository_storage', admin) expect(response).to have_gitlab_http_status(200) expect(json_response).to be_kind_of(Array) - expect(json_response.first['storage_name']).to eq('broken') - expect(json_response.first['failing_on_hosts']).to eq(['web01']) - expect(json_response.first['total_failures']).to eq(4) + expect(json_response).to be_empty end describe 'GET circuit_breakers/repository_storage/failing' do it 'returns an array of failing storages' do - expect(Gitlab::Git::Storage::Health).to receive(:for_failing_storages) do - [Gitlab::Git::Storage::Health.new('broken', [{ name: 'prefix:broken:web01', failure_count: 4 }])] - end - get api('/circuit_breakers/repository_storage/failing', admin) expect(response).to have_gitlab_http_status(200) expect(json_response).to be_kind_of(Array) + expect(json_response).to be_empty end end end describe 'DELETE circuit_breakers/repository_storage' do it 'clears all circuit_breakers' do - expect(Gitlab::Git::Storage::FailureInfo).to receive(:reset_all!) - delete api('/circuit_breakers/repository_storage', admin) expect(response).to have_gitlab_http_status(204) diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index e379bd9785a..fb1be16a111 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -2,11 +2,12 @@ require 'spec_helper' describe API::Settings, 'Settings' do let(:user) { create(:user) } - let(:admin) { create(:admin) } + set(:admin) { create(:admin) } describe "GET /application/settings" do it "returns application settings" do get api("/application/settings", admin) + expect(response).to have_gitlab_http_status(200) expect(json_response).to be_an Hash expect(json_response['default_projects_limit']).to eq(42) @@ -23,7 +24,6 @@ describe API::Settings, 'Settings' do expect(json_response['dsa_key_restriction']).to eq(0) expect(json_response['ecdsa_key_restriction']).to eq(0) expect(json_response['ed25519_key_restriction']).to eq(0) - expect(json_response['circuitbreaker_failure_count_threshold']).not_to be_nil expect(json_response['performance_bar_allowed_group_id']).to be_nil expect(json_response['instance_statistics_visibility_private']).to be(false) expect(json_response).not_to have_key('performance_bar_allowed_group_path') @@ -62,7 +62,6 @@ describe API::Settings, 'Settings' do dsa_key_restriction: 2048, ecdsa_key_restriction: 384, ed25519_key_restriction: 256, - circuitbreaker_check_interval: 2, enforce_terms: true, terms: 'Hello world!', performance_bar_allowed_group_path: group.full_path, @@ -88,7 +87,6 @@ describe API::Settings, 'Settings' do expect(json_response['dsa_key_restriction']).to eq(2048) expect(json_response['ecdsa_key_restriction']).to eq(384) expect(json_response['ed25519_key_restriction']).to eq(256) - expect(json_response['circuitbreaker_check_interval']).to eq(2) expect(json_response['enforce_terms']).to be(true) expect(json_response['terms']).to eq('Hello world!') expect(json_response['performance_bar_allowed_group_id']).to eq(group.id) diff --git a/spec/support/stored_repositories.rb b/spec/support/stored_repositories.rb index 26f823cb6ef..6a9ad43941d 100644 --- a/spec/support/stored_repositories.rb +++ b/spec/support/stored_repositories.rb @@ -7,24 +7,5 @@ RSpec.configure do |config| allow(Gitlab::GitalyClient).to receive(:call) do raise GRPC::Unavailable.new('Gitaly broken in this spec') end - - # Track the maximum number of failures - first_failure = Time.parse("2017-11-14 17:52:30") - last_failure = Time.parse("2017-11-14 18:54:37") - failure_count = Gitlab::CurrentSettings.circuitbreaker_failure_count_threshold + 1 - cache_key = "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}broken:#{Gitlab::Environment.hostname}" - - Gitlab::Git::Storage.redis.with do |redis| - redis.pipelined do - redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key) - redis.hset(cache_key, :first_failure, first_failure.to_i) - redis.hset(cache_key, :last_failure, last_failure.to_i) - redis.hset(cache_key, :failure_count, failure_count.to_i) - end - end - end - - config.after(:each, :broken_storage) do - Gitlab::Git::Storage.redis.with(&:flushall) end end |