Commit c365dea8 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Don't use `Redis#keys` in the circuitbreaker

parent a854431c
---
title: Forbid the usage of `Redis#keys`
merge_request: 14889
author:
type: fixed
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
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
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