From c365dea887ae0f139cf99e36701a2e4ca08ec0b9 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 16 Oct 2017 11:38:01 +0200 Subject: Don't use `Redis#keys` in the circuitbreaker --- .../unreleased/bvl-do-not-use-redis-keys.yml | 5 ++++ lib/gitlab/git/storage/circuit_breaker.rb | 2 +- lib/gitlab/git/storage/health.rb | 20 +++++++++++---- .../lib/gitlab/git/storage/circuit_breaker_spec.rb | 4 +++ spec/lib/gitlab/git/storage/health_spec.rb | 30 ---------------------- spec/support/redis_without_keys.rb | 8 ++++++ 6 files changed, 33 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/bvl-do-not-use-redis-keys.yml create mode 100644 spec/support/redis_without_keys.rb diff --git a/changelogs/unreleased/bvl-do-not-use-redis-keys.yml b/changelogs/unreleased/bvl-do-not-use-redis-keys.yml new file mode 100644 index 00000000000..f703aad2065 --- /dev/null +++ b/changelogs/unreleased/bvl-do-not-use-redis-keys.yml @@ -0,0 +1,5 @@ +--- +title: Forbid the usage of `Redis#keys` +merge_request: 14889 +author: +type: fixed diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb index 1eaa2d83fb6..ba56aa2baf7 100644 --- a/lib/gitlab/git/storage/circuit_breaker.rb +++ b/lib/gitlab/git/storage/circuit_breaker.rb @@ -18,7 +18,7 @@ module Gitlab pattern = "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}*" Gitlab::Git::Storage.redis.with do |redis| - all_storage_keys = redis.keys(pattern) + all_storage_keys = redis.scan_each(match: pattern).to_a redis.del(*all_storage_keys) unless all_storage_keys.empty? end diff --git a/lib/gitlab/git/storage/health.rb b/lib/gitlab/git/storage/health.rb index 1564e94b7f7..7049772fe3b 100644 --- a/lib/gitlab/git/storage/health.rb +++ b/lib/gitlab/git/storage/health.rb @@ -23,26 +23,36 @@ module Gitlab end end - def self.all_keys_for_storages(storage_names, redis) + private_class_method def self.all_keys_for_storages(storage_names, redis) keys_per_storage = {} redis.pipelined do storage_names.each do |storage_name| pattern = pattern_for_storage(storage_name) + matched_keys = redis.scan_each(match: pattern) - keys_per_storage[storage_name] = redis.keys(pattern) + keys_per_storage[storage_name] = matched_keys end end - keys_per_storage + # We need to make sure each lazy-loaded `Enumerator` for matched keys + # is loaded into an array. + # + # Otherwise it would be loaded in the second `Redis#pipelined` block + # within `.load_for_keys`. In this pipelined call, the active + # Redis-client changes again, so the values would not be available + # until the end of that pipelined-block. + keys_per_storage.each do |storage_name, key_future| + keys_per_storage[storage_name] = key_future.to_a + end end - def self.load_for_keys(keys_per_storage, redis) + private_class_method def self.load_for_keys(keys_per_storage, redis) info_for_keys = {} redis.pipelined do keys_per_storage.each do |storage_name, keys_future| - info_for_storage = keys_future.value.map do |key| + info_for_storage = keys_future.map do |key| { name: key, failure_count: redis.hget(key, :failure_count) } end diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb index 98cf7966dad..4edd360d11d 100644 --- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb +++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb @@ -49,6 +49,10 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: 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 '.for_storage' do diff --git a/spec/lib/gitlab/git/storage/health_spec.rb b/spec/lib/gitlab/git/storage/health_spec.rb index 2d3af387971..4a14a5201d1 100644 --- a/spec/lib/gitlab/git/storage/health_spec.rb +++ b/spec/lib/gitlab/git/storage/health_spec.rb @@ -20,36 +20,6 @@ describe Gitlab::Git::Storage::Health, clean_gitlab_redis_shared_state: true, br end end - describe '.load_for_keys' do - let(:subject) do - results = Gitlab::Git::Storage.redis.with do |redis| - fake_future = double - allow(fake_future).to receive(:value).and_return([host1_key]) - described_class.load_for_keys({ 'broken' => fake_future }, redis) - end - - # Make sure the `Redis#future is loaded - results.inject({}) do |result, (name, info)| - info.each { |i| i[:failure_count] = i[:failure_count].value.to_i } - - result[name] = info - - result - end - end - - it 'loads when there is no info in redis' do - expect(subject).to eq('broken' => [{ name: host1_key, failure_count: 0 }]) - end - - it 'reads the correct values for a storage from redis' do - set_in_redis(host1_key, 5) - set_in_redis(host2_key, 7) - - expect(subject).to eq('broken' => [{ name: host1_key, failure_count: 5 }]) - end - end - describe '.for_all_storages' do it 'loads health status for all configured storages' do healths = described_class.for_all_storages diff --git a/spec/support/redis_without_keys.rb b/spec/support/redis_without_keys.rb new file mode 100644 index 00000000000..6220167dee6 --- /dev/null +++ b/spec/support/redis_without_keys.rb @@ -0,0 +1,8 @@ +class Redis + ForbiddenCommand = Class.new(StandardError) + + def keys(*args) + raise ForbiddenCommand.new("Don't use `Redis#keys` as it iterates over all "\ + "keys in redis. Use `Redis#scan_each` instead.") + end +end -- cgit v1.2.1