+
### DingTalk OmniAuth provider
diff --git a/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl.rb b/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl.rb
index 2672498b627..7886abdb6d4 100644
--- a/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl.rb
+++ b/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl.rb
@@ -14,7 +14,7 @@ module Gitlab
ttl_jitter = 2.hours.to_i
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
keys.each { |key| pipeline.expire(key, ttl_duration + rand(-ttl_jitter..ttl_jitter)) }
end
end
@@ -25,7 +25,7 @@ module Gitlab
end
def redis
- @redis ||= ::Redis.new(Gitlab::Redis::Cache.params)
+ @redis ||= Gitlab::Redis::Cache.redis
end
end
end
diff --git a/lib/gitlab/cache/import/caching.rb b/lib/gitlab/cache/import/caching.rb
index c538e5162c7..0b24137076d 100644
--- a/lib/gitlab/cache/import/caching.rb
+++ b/lib/gitlab/cache/import/caching.rb
@@ -139,7 +139,7 @@ module Gitlab
key = cache_key_for(raw_key)
with_redis do |redis|
- redis.sismember(key, value)
+ redis.sismember(key, value || value.to_s)
end
end
@@ -162,7 +162,7 @@ module Gitlab
def self.write_multiple(mapping, key_prefix: nil, timeout: TIMEOUT)
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
mapping.each do |raw_key, value|
key = cache_key_for("#{key_prefix}#{raw_key}")
diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index dc5f4e1b324..0d5a66175f8 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -197,7 +197,9 @@ module Gitlab
record_hit_ratio(results)
results.map! do |result|
- Gitlab::Json.parse(gzip_decompress(result), symbolize_names: true) unless result.nil?
+ unless result.nil?
+ Gitlab::Json.parse(gzip_decompress(result.force_encoding(Encoding::UTF_8)), symbolize_names: true)
+ end
end
file_paths.zip(results).to_h
diff --git a/lib/gitlab/discussions_diff/highlight_cache.rb b/lib/gitlab/discussions_diff/highlight_cache.rb
index 18ff7c28e17..dbdc8c3c95c 100644
--- a/lib/gitlab/discussions_diff/highlight_cache.rb
+++ b/lib/gitlab/discussions_diff/highlight_cache.rb
@@ -16,7 +16,7 @@ module Gitlab
def write_multiple(mapping)
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipelined|
+ redis.pipelined do |pipelined|
mapping.each do |raw_key, value|
key = cache_key_for(raw_key)
@@ -42,7 +42,7 @@ module Gitlab
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if Gitlab::Redis::ClusterUtil.cluster?(redis)
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
keys.each { |key| pipeline.get(key) }
end
else
@@ -54,7 +54,7 @@ module Gitlab
content.map! do |lines|
next unless lines
- Gitlab::Json.parse(gzip_decompress(lines)).map! do |line|
+ Gitlab::Json.parse(gzip_decompress(lines.force_encoding(Encoding::UTF_8))).map! do |line|
Gitlab::Diff::Line.safe_init_from_hash(line)
end
end
diff --git a/lib/gitlab/etag_caching/store.rb b/lib/gitlab/etag_caching/store.rb
index 5fdf5ac9436..57c160c9607 100644
--- a/lib/gitlab/etag_caching/store.rb
+++ b/lib/gitlab/etag_caching/store.rb
@@ -17,7 +17,7 @@ module Gitlab
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
with_redis do |redis|
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
keys.each_with_index do |key, i|
pipeline.set(redis_shared_state_key(key), etags[i], ex: EXPIRY_TIME, nx: only_if_missing)
end
diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb
index d470fb503cb..b5969560774 100644
--- a/lib/gitlab/exclusive_lease.rb
+++ b/lib/gitlab/exclusive_lease.rb
@@ -62,6 +62,7 @@ module Gitlab
def self.cancel(key, uuid)
return unless key.present?
+ return unless uuid.present?
Gitlab::Redis::SharedState.with do |redis|
redis.eval(LUA_CANCEL_SCRIPT, keys: [ensure_prefixed_key(key)], argv: [uuid])
@@ -142,7 +143,7 @@ module Gitlab
# false if the lease is taken by a different UUID or inexistent.
def renew
Gitlab::Redis::SharedState.with do |redis|
- result = redis.eval(LUA_RENEW_SCRIPT, keys: [@redis_shared_state_key], argv: [@uuid, @timeout])
+ result = redis.eval(LUA_RENEW_SCRIPT, keys: [@redis_shared_state_key], argv: [@uuid, @timeout.to_i])
result == @uuid
end
end
diff --git a/lib/gitlab/instrumentation/redis_cluster_validator.rb b/lib/gitlab/instrumentation/redis_cluster_validator.rb
index 948132e6edd..5c3655f8f80 100644
--- a/lib/gitlab/instrumentation/redis_cluster_validator.rb
+++ b/lib/gitlab/instrumentation/redis_cluster_validator.rb
@@ -1,7 +1,7 @@
# frozen_string_literal: true
require 'rails'
-require 'redis'
+require 'redis-clustering'
module Gitlab
module Instrumentation
@@ -230,7 +230,7 @@ module Gitlab
end
def key_slot(key)
- ::Redis::Cluster::KeySlotConverter.convert(extract_hash_tag(key))
+ ::RedisClient::Cluster::KeySlotConverter.convert(extract_hash_tag(key))
end
# This is almost identical to Redis::Cluster::Command#extract_hash_tag,
diff --git a/lib/gitlab/instrumentation/redis_interceptor.rb b/lib/gitlab/instrumentation/redis_interceptor.rb
deleted file mode 100644
index 9c89af6a0dc..00000000000
--- a/lib/gitlab/instrumentation/redis_interceptor.rb
+++ /dev/null
@@ -1,55 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Instrumentation
- module RedisInterceptor
- include RedisHelper
-
- def call(command)
- instrument_call([command], instrumentation_class) do
- super
- end
- end
-
- def call_pipeline(pipeline)
- instrument_call(pipeline.commands, instrumentation_class, true) do
- super
- end
- end
-
- def write(command)
- measure_write_size(command, instrumentation_class) if ::RequestStore.active?
- super
- end
-
- def read
- result = super
- measure_read_size(result, instrumentation_class) if ::RequestStore.active?
- result
- end
-
- def ensure_connected
- super do
- instrument_reconnection_errors do
- yield
- end
- end
- end
-
- def instrument_reconnection_errors
- yield
- rescue ::Redis::BaseConnectionError => ex
- instrumentation_class.instance_count_connection_exception(ex)
-
- raise ex
- end
-
- # That's required so it knows which GitLab Redis instance
- # it's interacting with in order to categorize accordingly.
- #
- def instrumentation_class
- @options[:instrumentation_class] # rubocop:disable Gitlab/ModuleWithInstanceVariables
- end
- end
- end
-end
diff --git a/lib/gitlab/issues/rebalancing/state.rb b/lib/gitlab/issues/rebalancing/state.rb
index c60dac6f571..12cc5f6e5dd 100644
--- a/lib/gitlab/issues/rebalancing/state.rb
+++ b/lib/gitlab/issues/rebalancing/state.rb
@@ -100,7 +100,7 @@ module Gitlab
def refresh_keys_expiration
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
pipeline.expire(issue_ids_key, REDIS_EXPIRY_TIME)
pipeline.expire(current_index_key, REDIS_EXPIRY_TIME)
pipeline.expire(current_project_key, REDIS_EXPIRY_TIME)
diff --git a/lib/gitlab/markdown_cache/redis/store.rb b/lib/gitlab/markdown_cache/redis/store.rb
index af9098c3300..85aeaba92c4 100644
--- a/lib/gitlab/markdown_cache/redis/store.rb
+++ b/lib/gitlab/markdown_cache/redis/store.rb
@@ -11,7 +11,7 @@ module Gitlab
data = Gitlab::Redis::Cache.with do |r|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- Gitlab::Redis::CrossSlot::Pipeline.new(r).pipelined do |pipeline|
+ r.pipelined do |pipeline|
subjects.each do |subject|
new(subject).read(pipeline)
end
diff --git a/lib/gitlab/patch/command_loader.rb b/lib/gitlab/patch/command_loader.rb
deleted file mode 100644
index 357b6270b0d..00000000000
--- a/lib/gitlab/patch/command_loader.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Patch
- module CommandLoader
- extend ActiveSupport::Concern
-
- class_methods do
- # Shuffle the node list to spread out initial connection creation amongst all nodes
- #
- # The input is a Redis::Cluster::Node instance which is an Enumerable.
- # `super` receives an Array of Redis::Client instead of a Redis::Cluster::Node
- def load(nodes)
- super(nodes.to_a.shuffle)
- end
- end
- end
- end
-end
diff --git a/lib/gitlab/patch/node_loader.rb b/lib/gitlab/patch/node_loader.rb
deleted file mode 100644
index 85237abc137..00000000000
--- a/lib/gitlab/patch/node_loader.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-# frozen_string_literal: true
-
-# Patch to address https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/2212#note_1287996694
-# It uses hostname instead of IP address if the former is present in `CLUSTER NODES` output.
-if Gem::Version.new(Redis::VERSION) > Gem::Version.new('4.8.1')
- raise 'New version of redis detected, please remove or update this patch'
-end
-
-module Gitlab
- module Patch
- module NodeLoader
- extend ActiveSupport::Concern
-
- class_methods do
- # Shuffle the node list to spread out initial connection creation amongst all nodes
- #
- # The input is a Redis::Cluster::Node instance which is an Enumerable.
- # `super` receives an Array of Redis::Client instead of a Redis::Cluster::Node
- def load_flags(nodes)
- super(nodes.to_a.shuffle)
- end
- end
-
- def self.prepended(base)
- base.class_eval do
- # monkey-patches https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/cluster/node_loader.rb#L23
- def self.fetch_node_info(node)
- node.call(%i[cluster nodes]).split("\n").map(&:split).to_h do |arr|
- [
- extract_host_identifier(arr[1]),
- (arr[2].split(',') & %w[master slave]).first # rubocop:disable Naming/InclusiveLanguage
- ]
- end
- end
-
- # Since `CLUSTER SLOT` uses the preferred endpoint determined by
- # the `cluster-preferred-endpoint-type` config value, we will prefer hostname over IP address.
- # See https://redis.io/commands/cluster-nodes/ for details on the output format.
- #
- # @param [String] Address info matching fhe format:
- def self.extract_host_identifier(node_address)
- ip_chunk, hostname, _auxiliaries = node_address.split(',')
- return ip_chunk.split('@').first if hostname.blank?
-
- port = ip_chunk.split('@').first.split(':')[1]
- "#{hostname}:#{port}"
- end
- end
- end
- end
- end
-end
diff --git a/lib/gitlab/patch/redis_cache_store.rb b/lib/gitlab/patch/redis_cache_store.rb
index 96729056ce5..35c617ba72b 100644
--- a/lib/gitlab/patch/redis_cache_store.rb
+++ b/lib/gitlab/patch/redis_cache_store.rb
@@ -20,7 +20,7 @@ module Gitlab
delete_count = 0
redis.with do |conn|
entries.each_slice(pipeline_batch_size) do |subset|
- delete_count += Gitlab::Redis::CrossSlot::Pipeline.new(conn).pipelined do |pipeline|
+ delete_count += conn.pipelined do |pipeline|
subset.each { |entry| pipeline.del(entry) }
end.sum
end
@@ -58,7 +58,7 @@ module Gitlab
def pipeline_mget(conn, keys)
keys.each_slice(pipeline_batch_size).flat_map do |subset|
- Gitlab::Redis::CrossSlot::Pipeline.new(conn).pipelined do |p|
+ conn.pipelined do |p|
subset.each { |key| p.get(key) }
end
end
diff --git a/lib/gitlab/patch/redis_cluster.rb b/lib/gitlab/patch/redis_cluster.rb
deleted file mode 100644
index 145ce35a317..00000000000
--- a/lib/gitlab/patch/redis_cluster.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-# Patch to expose `find_node_key` method for cross-slot pipelining
-# In redis v5.0.x, cross-slot pipelining is implemented via redis-cluster-client.
-# This patch should be removed since there is no need for it.
-# Gitlab::Redis::CrossSlot and its usage should be removed as well.
-if Gem::Version.new(Redis::VERSION) != Gem::Version.new('4.8.0')
- raise 'New version of redis detected, please remove or update this patch'
-end
-
-module Gitlab
- module Patch
- module RedisCluster
- # _find_node_key exposes a private function of the same name in Redis::Cluster.
- # See https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/cluster.rb#L282
- def _find_node_key(command)
- find_node_key(command)
- end
- end
- end
-end
diff --git a/lib/gitlab/patch/redis_store_factory.rb b/lib/gitlab/patch/redis_store_factory.rb
new file mode 100644
index 00000000000..81636ed665d
--- /dev/null
+++ b/lib/gitlab/patch/redis_store_factory.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Patch
+ module RedisStoreFactory
+ def create
+ # rubocop:disable Gitlab/ModuleWithInstanceVariables -- patched code references @options in redis-store
+ opt = @options
+ # rubocop:enable Gitlab/ModuleWithInstanceVariables
+ return Gitlab::Redis::ClusterStore.new(opt) if opt[:nodes]
+
+ super
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/patch/slot_loader.rb b/lib/gitlab/patch/slot_loader.rb
deleted file mode 100644
index e302d844078..00000000000
--- a/lib/gitlab/patch/slot_loader.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Patch
- module SlotLoader
- extend ActiveSupport::Concern
-
- class_methods do
- # Shuffle the node list to spread out initial connection creation amongst all nodes
- #
- # The input is a Redis::Cluster::Node instance which is an Enumerable.
- # `super` receives an Array of Redis::Client instead of a Redis::Cluster::Node
- def load(nodes)
- super(nodes.to_a.shuffle)
- end
- end
- end
- end
-end
diff --git a/lib/gitlab/redis/cluster_store.rb b/lib/gitlab/redis/cluster_store.rb
new file mode 100644
index 00000000000..d660c782d4d
--- /dev/null
+++ b/lib/gitlab/redis/cluster_store.rb
@@ -0,0 +1,81 @@
+# frozen_string_literal: true
+
+require 'redis-clustering'
+require 'redis/store/ttl'
+require 'redis/store/interface'
+require 'redis/store/namespace'
+require 'redis/store/serialization'
+
+module Gitlab
+ module Redis
+ class ClusterStore < ::Redis::Cluster
+ include ::Redis::Store::Interface
+
+ def initialize(options = {})
+ orig_options = options.dup
+
+ @serializer = orig_options.key?(:serializer) ? orig_options.delete(:serializer) : Marshal
+
+ unless orig_options[:marshalling].nil?
+ # `marshalling` only used here, might not be supported in `super`
+ @serializer = orig_options.delete(:marshalling) ? Marshal : nil
+ end
+
+ _remove_unsupported_options(options)
+ super(options)
+
+ _extend_marshalling
+ _extend_namespace orig_options
+ end
+
+ # copies ::Redis::Store::Ttl implementation in a redis-v5 compatible manner
+ def set(key, value, options = nil)
+ ttl = get_ttl(options)
+ if ttl
+ setex(key, ttl.to_i, value, raw: true)
+ else
+ super(key, value)
+ end
+ end
+
+ # copies ::Redis::Store::Ttl implementation in a redis-v5 compatible manner
+ def setnx(key, value, options = nil)
+ ttl = get_ttl(options)
+ if ttl
+ multi do |m|
+ m.setnx(key, value)
+ m.expire(key, ttl)
+ end
+ else
+ super(key, value)
+ end
+ end
+
+ private
+
+ def get_ttl(options)
+ # https://github.com/redis-store/redis-store/blob/v1.10.0/lib/redis/store/ttl.rb#L37
+ options[:expire_after] || options[:expires_in] || options[:expire_in] if options
+ end
+
+ def _remove_unsupported_options(options)
+ # Unsupported keywords should be removed to avoid errors
+ # https://github.com/redis-rb/redis-client/blob/v0.13.0/lib/redis_client/config.rb#L21
+ options.delete(:raw)
+ options.delete(:serializer)
+ options.delete(:marshalling)
+ options.delete(:namespace)
+ options.delete(:scheme)
+ end
+
+ def _extend_marshalling
+ extend ::Redis::Store::Serialization unless @serializer.nil?
+ end
+
+ def _extend_namespace(options)
+ @namespace = options[:namespace]
+ extend ::Redis::Store::Namespace
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/redis/cluster_util.rb b/lib/gitlab/redis/cluster_util.rb
index 9e307940de3..99f337749d0 100644
--- a/lib/gitlab/redis/cluster_util.rb
+++ b/lib/gitlab/redis/cluster_util.rb
@@ -13,14 +13,14 @@ module Gitlab
if obj.is_a?(MultiStore)
cluster?(obj.primary_store) || cluster?(obj.secondary_store)
else
- obj.respond_to?(:_client) && obj._client.is_a?(::Redis::Cluster)
+ obj.is_a?(::Redis::Cluster)
end
end
def batch_unlink(keys, redis)
expired_count = 0
keys.each_slice(1000) do |subset|
- expired_count += Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ expired_count += redis.pipelined do |pipeline|
subset.each { |key| pipeline.unlink(key) }
end.sum
end
@@ -30,7 +30,7 @@ module Gitlab
# Redis cluster alternative to mget
def batch_get(keys, redis)
keys.each_slice(1000).flat_map do |subset|
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
subset.map { |key| pipeline.get(key) }
end
end
diff --git a/lib/gitlab/redis/command_builder.rb b/lib/gitlab/redis/command_builder.rb
new file mode 100644
index 00000000000..99d26760b3d
--- /dev/null
+++ b/lib/gitlab/redis/command_builder.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Redis
+ module CommandBuilder
+ extend self
+
+ # Ref: https://github.com/redis-rb/redis-client/blob/v0.19.1/lib/redis_client/command_builder.rb
+ # we modify the command builder to convert nil to strings as this behaviour was present in
+ # https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/connection/command_helper.rb#L20
+ #
+ # Note that we only adopt the Ruby3.x-compatible logic in .generate.
+ # Symbol.method_defined?(:name) is true in Ruby 3
+ def generate(args, kwargs = nil)
+ command = args.flat_map do |element|
+ case element
+ when Hash
+ element.flatten
+ else
+ element
+ end
+ end
+
+ kwargs&.each do |key, value|
+ if value
+ if value == true
+ command << key.name
+ else
+ command << key.name << value
+ end
+ end
+ end
+
+ command.map! do |element|
+ case element
+ when String
+ element
+ when Symbol
+ element.name
+ when Integer, Float, NilClass
+ element.to_s
+ else
+ raise TypeError, "Unsupported command argument type: #{element.class}"
+ end
+ end
+
+ raise ArgumentError, "can't issue an empty redis command" if command.empty?
+
+ command
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/redis/cross_slot.rb b/lib/gitlab/redis/cross_slot.rb
deleted file mode 100644
index e5aa6d9ce72..00000000000
--- a/lib/gitlab/redis/cross_slot.rb
+++ /dev/null
@@ -1,141 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Redis
- module CrossSlot
- class Router
- attr_reader :node_mapping, :futures, :node_sequence, :cmd_queue
-
- delegate :respond_to_missing?, to: :@redis
-
- # This map contains redis-rb methods which does not map directly
- # to a standard Redis command. It is used transform unsupported commands to standard commands
- # to find the node key for unsupported commands.
- #
- # Redis::Cluster::Command only contains details of commands which the Redis Server
- # returns. Hence, commands like mapped_hmget and hscan_each internally will call the
- # base command, hmget and hscan respectively.
- #
- # See https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/cluster/command.rb
- UNSUPPORTED_CMD_MAPPING = {
- # Internally, redis-rb calls the supported Redis command and transforms the output.
- # See https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/commands/hashes.rb#L104
- mapped_hmget: :hmget
- }.freeze
-
- # Initializes the CrossSlot::Router
- # @param {::Redis}
- def initialize(redis)
- @redis = redis
- @node_mapping = {}
- @futures = {}
- @node_sequence = []
- @cmd_queue = []
- end
-
- # For now we intercept every redis.call and return a Gitlab-Future object.
- # This method groups every commands to a node for fan-out. Commands are grouped using the first key.
- #
- # rubocop:disable Style/MissingRespondToMissing
- def method_missing(cmd, *args, **kwargs, &blk)
- # Note that we can re-map the command without affecting execution as it is
- # solely for finding the node key. The original cmd will be executed.
- node = @redis._client._find_node_key([UNSUPPORTED_CMD_MAPPING.fetch(cmd, cmd)] + args)
-
- @node_mapping[node] ||= []
- @futures[node] ||= []
-
- @node_sequence << node
- @node_mapping[node] << [cmd, args, kwargs || {}, blk]
- f = Future.new
- @futures[node] << f
- @cmd_queue << [f, cmd, args, kwargs || {}, blk]
- f
- end
- # rubocop:enable Style/MissingRespondToMissing
- end
-
- # Wraps over redis-rb's Future in
- # https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/pipeline.rb#L244
- class Future
- def set(future, is_val = false)
- @redis_future = future
- @is_val = is_val
- end
-
- def value
- return @redis_val if @is_val
-
- @redis_future.value
- end
- end
-
- # Pipeline allows cross-slot pipelined to be called. The fan-out logic is implemented in
- # https://github.com/redis-rb/redis-cluster-client/blob/master/lib/redis_client/cluster/pipeline.rb
- # which is available in redis-rb v5.0.
- #
- # This file can be deprecated after redis-rb v4.8.0 is upgraded to v5.0
- class Pipeline
- # Initializes the CrossSlot::Pipeline
- # @param {::Redis}
- def initialize(redis)
- @redis = redis
- end
-
- # pipelined is used in place of ::Redis `.pipelined` when running in a cluster context
- # where cross-slot operations may happen.
- def pipelined(&block)
- # Directly call .pipelined and defer the pipeline execution to MultiStore.
- # MultiStore could wrap over 0, 1, or 2 Redis Cluster clients, handling it here
- # will not work for 2 clients since the key-slot topology can differ.
- if use_cross_slot_pipelining?
- router = Router.new(@redis)
- yield router
- execute_commands(router)
- else
- # use redis-rb's pipelined method
- @redis.pipelined(&block)
- end
- end
-
- private
-
- def use_cross_slot_pipelining?
- !@redis.instance_of?(::Gitlab::Redis::MultiStore) && @redis._client.instance_of?(::Redis::Cluster)
- end
-
- def execute_commands(router)
- router.node_mapping.each do |node_key, commands|
- # TODO possibly use Threads to speed up but for now `n` is 3-5 which is small.
- @redis.pipelined do |p|
- commands.each_with_index do |command, idx|
- future = router.futures[node_key][idx]
- cmd, args, kwargs, blk = command
- future.set(p.public_send(cmd, *args, **kwargs, &blk)) # rubocop:disable GitlabSecurity/PublicSend
- end
- end
- end
-
- router.node_sequence.map do |node_key|
- router.futures[node_key].shift.value
- end
- rescue ::Redis::CommandError => err
- if err.message.start_with?('MOVED', 'ASK')
- Gitlab::ErrorTracking.log_exception(err)
- return execute_commands_sequentially(router)
- end
-
- raise
- end
-
- def execute_commands_sequentially(router)
- router.cmd_queue.map do |command|
- future, cmd, args, kwargs, blk = command
- future.set(@redis.public_send(cmd, *args, **kwargs, &blk), true) # rubocop:disable GitlabSecurity/PublicSend
- future.value
- end
- end
- end
- end
- end
-end
diff --git a/lib/gitlab/redis/multi_store.rb b/lib/gitlab/redis/multi_store.rb
index e6262f7f61b..476077cb96f 100644
--- a/lib/gitlab/redis/multi_store.rb
+++ b/lib/gitlab/redis/multi_store.rb
@@ -432,16 +432,6 @@ module Gitlab
# rubocop:disable GitlabSecurity/PublicSend
def send_command(redis_instance, command_name, *args, **kwargs, &block)
- # Run wrapped pipeline for each instance individually so that the fan-out is distinct.
- # If both primary and secondary are Redis Clusters, the slot-node distribution could
- # be different.
- #
- # We ignore args and kwargs since `pipelined` does not accept arguments
- # See https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis.rb#L164
- if command_name.to_s == 'pipelined' && redis_instance._client.instance_of?(::Redis::Cluster)
- return Gitlab::Redis::CrossSlot::Pipeline.new(redis_instance).pipelined(&block)
- end
-
if block
# Make sure that block is wrapped and executed only on the redis instance that is executing the block
redis_instance.send(command_name, *args, **kwargs) do |*params|
@@ -462,7 +452,7 @@ module Gitlab
end
def redis_store?(pool)
- pool.with { |c| c.instance_of?(Gitlab::Redis::MultiStore) || c.is_a?(::Redis) }
+ pool.with { |c| c.instance_of?(Gitlab::Redis::MultiStore) || c.is_a?(::Redis) || c.is_a?(::Redis::Cluster) }
end
def validate_stores!
diff --git a/lib/gitlab/redis/wrapper.rb b/lib/gitlab/redis/wrapper.rb
index a106ed3884e..f7a7e703d90 100644
--- a/lib/gitlab/redis/wrapper.rb
+++ b/lib/gitlab/redis/wrapper.rb
@@ -20,7 +20,7 @@ module Gitlab
CommandExecutionError = Class.new(StandardError)
class << self
- delegate :params, :url, :store, :encrypted_secrets, :redis_client_params, to: :new
+ delegate :params, :url, :store, :encrypted_secrets, to: :new
def with
pool.with { |redis| yield redis }
@@ -90,7 +90,17 @@ module Gitlab
end
def redis
- ::Redis.new(params)
+ init_redis(params)
+ end
+
+ private
+
+ def init_redis(config)
+ if config[:nodes].present?
+ ::Redis::Cluster.new(config.merge({ concurrency: { model: :none } }))
+ else
+ ::Redis.new(config)
+ end
end
end
@@ -99,13 +109,8 @@ module Gitlab
end
def params
- redis_store_options
- end
-
- # redis_client_params modifies redis_store_options to be compatible with redis-client
- # TODO: when redis-rb is updated to v5, there is no need to support 2 types of config format
- def redis_client_params
options = redis_store_options
+ options[:command_builder] = CommandBuilder
# avoid passing classes into options as Sidekiq scrubs the options with Marshal.dump + Marshal.load
# ref https://github.com/sidekiq/sidekiq/blob/v7.1.6/lib/sidekiq/redis_connection.rb#L37
@@ -114,14 +119,14 @@ module Gitlab
# we use strings to look up Gitlab::Instrumentation::Redis.storage_hash as a bypass
options[:custom] = { instrumentation_class: self.class.store_name }
- # TODO: add support for cluster when upgrading to redis-rb v5.y.z we do not need cluster support
- # as Sidekiq workload should not and does not run in a Redis Cluster
- # support to be added in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134862
if options[:sentinels]
# name is required in RedisClient::SentinelConfig
# https://github.com/redis-rb/redis-client/blob/1ab081c1d0e47df5d55e011c9390c70b2eef6731/lib/redis_client/sentinel_config.rb#L17
options[:name] = options[:host]
options.except(:scheme, :instrumentation_class, :host, :port)
+ elsif options[:cluster]
+ options[:nodes] = options[:cluster].map { |c| c.except(:scheme) }
+ options.except(:scheme, :instrumentation_class, :cluster)
else
# remove disallowed keys as seen in
# https://github.com/redis-rb/redis-client/blob/1ab081c1d0e47df5d55e011c9390c70b2eef6731/lib/redis_client/config.rb#L21
@@ -134,7 +139,7 @@ module Gitlab
end
def db
- redis_store_options[:db]
+ redis_store_options[:db] || 0
end
def sentinels
@@ -156,7 +161,7 @@ module Gitlab
end
def store(extras = {})
- ::Redis::Store::Factory.create(redis_store_options.merge(extras))
+ ::Redis::Store::Factory.create(params.merge(extras))
end
def encrypted_secrets
@@ -182,7 +187,11 @@ module Gitlab
final_config = parse_extra_config(decrypted_config)
result = if final_config[:cluster].present?
- final_config[:db] = 0 # Redis Cluster only supports db 0
+ final_config[:cluster] = final_config[:cluster].map do |node|
+ next node unless node.is_a?(String)
+
+ ::Redis::Store::Factory.extract_host_options_from_uri(node)
+ end
final_config
else
parse_redis_url(final_config)
diff --git a/lib/gitlab/repository_hash_cache.rb b/lib/gitlab/repository_hash_cache.rb
index fab0e9e09e8..6da5556833f 100644
--- a/lib/gitlab/repository_hash_cache.rb
+++ b/lib/gitlab/repository_hash_cache.rb
@@ -86,6 +86,8 @@ module Gitlab
full_key = cache_key(key)
+ hash = standardize_hash(hash)
+
with do |redis|
results = redis.pipelined do |pipeline|
# Set each hash key to the provided value
diff --git a/lib/gitlab/set_cache.rb b/lib/gitlab/set_cache.rb
index eb73a0a3d31..967a139e7fb 100644
--- a/lib/gitlab/set_cache.rb
+++ b/lib/gitlab/set_cache.rb
@@ -62,7 +62,7 @@ module Gitlab
with do |redis|
redis.multi do |multi|
- multi.sismember(full_key, value)
+ multi.sismember(full_key, value.to_s)
multi.exists?(full_key) # rubocop:disable CodeReuse/ActiveRecord
end
end
diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb
index 883e1ba0558..c40f125db40 100644
--- a/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb
+++ b/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb
@@ -75,7 +75,7 @@ module Gitlab
argv = []
job_wal_locations.each do |connection_name, location|
diff = pg_wal_lsn_diff(connection_name)
- argv += [connection_name, diff || '', location]
+ argv += [connection_name, diff ? diff.to_f : '', location]
end
with_redis { |r| r.eval(UPDATE_WAL_COOKIE_SCRIPT, keys: [cookie_key], argv: argv) }
diff --git a/qa/gdk/Dockerfile.gdk b/qa/gdk/Dockerfile.gdk
index 0bb44b5b773..6cc2c6a3ed4 100644
--- a/qa/gdk/Dockerfile.gdk
+++ b/qa/gdk/Dockerfile.gdk
@@ -1,4 +1,4 @@
-ARG GDK_SHA=5dd3fde91e00da3e35c8358fd66e4cba8b97d46d
+ARG GDK_SHA=fb96a94cf54d4b0c5db2a3bdf905874dbbc1cf67
FROM registry.gitlab.com/gitlab-org/gitlab-development-kit/asdf-bootstrapped-verify/main:${GDK_SHA} as base
diff --git a/spec/controllers/concerns/product_analytics_tracking_spec.rb b/spec/controllers/concerns/product_analytics_tracking_spec.rb
index 7b48782be98..247be5429a1 100644
--- a/spec/controllers/concerns/product_analytics_tracking_spec.rb
+++ b/spec/controllers/concerns/product_analytics_tracking_spec.rb
@@ -109,7 +109,8 @@ RSpec.describe ProductAnalyticsTracking, :snowplow, feature_category: :product_a
end
it 'tracks total Redis counters' do
- expect(Gitlab::Usage::Metrics::Instrumentations::TotalCountMetric).to receive(:redis_key).twice # total and 7d
+ expect(Gitlab::Usage::Metrics::Instrumentations::TotalCountMetric).to receive(:redis_key)
+ .twice.and_call_original # total and 7d
get :index
end
diff --git a/spec/fixtures/config/redis_new_format_host_standalone.yml b/spec/fixtures/config/redis_new_format_host_standalone.yml
new file mode 100644
index 00000000000..f5836586247
--- /dev/null
+++ b/spec/fixtures/config/redis_new_format_host_standalone.yml
@@ -0,0 +1,8 @@
+# redis://[:password@]host[:port][/db-number][?option=value]
+# more details: http://www.iana.org/assignments/uri-schemes/prov/redis
+development:
+ url: redis://:mynewpassword@development-host:6379/99
+test:
+ url: redis://:mynewpassword@test-host:6379/99
+production:
+ url: redis://:mynewpassword@production-host:6379/99
diff --git a/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb b/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb
index cf82fd751dd..d89ad21ca82 100644
--- a/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb
+++ b/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb
@@ -27,7 +27,7 @@ RSpec.describe 'ActionCableSubscriptionAdapterIdentifier override' do
sub = ActionCable.server.pubsub.send(:redis_connection)
- expect(sub.connection[:id]).to eq('unix:///home/localuser/redis/redis.socket/0')
+ expect(sub.connection[:id]).to eq('/home/localuser/redis/redis.socket/0')
expect(ActionCable.server.config.cable[:id]).to be_nil
end
end
diff --git a/spec/initializers/session_store_spec.rb b/spec/initializers/session_store_spec.rb
index c9333d022dd..bd943830f6e 100644
--- a/spec/initializers/session_store_spec.rb
+++ b/spec/initializers/session_store_spec.rb
@@ -15,7 +15,7 @@ RSpec.describe 'Session initializer for GitLab' do
describe 'config#session_store' do
it 'initialized as a redis_store with a proper servers configuration' do
- expect(subject).to receive(:session_store).with(:redis_store, a_hash_including(redis_store: kind_of(::Redis::Store)))
+ expect(subject).to receive(:session_store).with(:redis_store, a_hash_including(redis_server: Gitlab::Redis::Sessions.params.merge(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE)))
load_session_store
end
diff --git a/spec/lib/click_house/migration_support/exclusive_lock_spec.rb b/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
index 5176cc75266..17e58a4ddef 100644
--- a/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
+++ b/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
@@ -5,6 +5,8 @@ require 'spec_helper'
RSpec.describe ClickHouse::MigrationSupport::ExclusiveLock, feature_category: :database do
include ExclusiveLeaseHelpers
+ let(:worker_id) { 1 }
+
let(:worker_class) do
# This worker will be active longer than the ClickHouse worker TTL
Class.new do
@@ -81,7 +83,7 @@ RSpec.describe ClickHouse::MigrationSupport::ExclusiveLock, feature_category: :d
end
around do |example|
- described_class.register_running_worker(worker_class, anything) do
+ described_class.register_running_worker(worker_class, worker_id) do
example.run
end
end
diff --git a/spec/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl_spec.rb b/spec/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl_spec.rb
index c52d1b4c9f2..a55ae5711a8 100644
--- a/spec/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl_spec.rb
+++ b/spec/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl_spec.rb
@@ -4,7 +4,7 @@ require 'spec_helper'
RSpec.describe Gitlab::BackgroundMigration::Redis::BackfillProjectPipelineStatusTtl,
:clean_gitlab_redis_cache, feature_category: :redis do
- let(:redis) { ::Redis.new(::Gitlab::Redis::Cache.params) }
+ let(:redis) { ::Gitlab::Redis::Cache.redis }
let(:keys) { ["cache:gitlab:project:1:pipeline_status", "cache:gitlab:project:2:pipeline_status"] }
let(:invalid_keys) { ["cache:gitlab:project:pipeline_status:1", "cache:gitlab:project:pipeline_status:2"] }
diff --git a/spec/lib/gitlab/database/migrations/runner_backoff/communicator_spec.rb b/spec/lib/gitlab/database/migrations/runner_backoff/communicator_spec.rb
index cfc3fb398e2..9f1ade529b1 100644
--- a/spec/lib/gitlab/database/migrations/runner_backoff/communicator_spec.rb
+++ b/spec/lib/gitlab/database/migrations/runner_backoff/communicator_spec.rb
@@ -28,7 +28,7 @@ RSpec.describe Gitlab::Database::Migrations::RunnerBackoff::Communicator, :clean
it 'reads from Redis' do
recorder = RedisCommands::Recorder.new { subject }
- expect(recorder.log).to include([:exists, 'gitlab:exclusive_lease:gitlab/database/migration/runner/backoff'])
+ expect(recorder.log).to include(['exists', 'gitlab:exclusive_lease:gitlab/database/migration/runner/backoff'])
end
context 'with runner_migrations_backoff disabled' do
diff --git a/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb b/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb
index 30981e4bd7d..8068be798d3 100644
--- a/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb
@@ -33,7 +33,7 @@ RSpec.describe Gitlab::DiscussionsDiff::HighlightCache, :clean_gitlab_redis_cach
mapping.each do |key, value|
full_key = described_class.cache_key_for(key)
- found_key = Gitlab::Redis::Cache.with { |r| r.get(full_key) }
+ found_key = Gitlab::Redis::Cache.with { |r| r.get(full_key).force_encoding("UTF-8") }
expect(described_class.gzip_decompress(found_key)).to eq(value.to_json)
end
diff --git a/spec/lib/gitlab/external_authorization/cache_spec.rb b/spec/lib/gitlab/external_authorization/cache_spec.rb
index 186bf7d7ec1..3dc23422e4e 100644
--- a/spec/lib/gitlab/external_authorization/cache_spec.rb
+++ b/spec/lib/gitlab/external_authorization/cache_spec.rb
@@ -23,9 +23,9 @@ RSpec.describe Gitlab::ExternalAuthorization::Cache, :clean_gitlab_redis_cache d
describe '#load' do
it 'reads stored info from redis' do
freeze_time do
- set_in_redis(:access, false)
+ set_in_redis(:access, false.to_s)
set_in_redis(:reason, 'Access denied for now')
- set_in_redis(:refreshed_at, Time.now)
+ set_in_redis(:refreshed_at, Time.now.to_s)
access, reason, refreshed_at = cache.load
diff --git a/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb b/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
index a8bded69696..2eb77add2ed 100644
--- a/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
+++ b/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
@@ -9,10 +9,9 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
include RedisHelpers
let_it_be(:redis_store_class) { define_helper_redis_store_class }
- let_it_be(:redis_client) { RedisClient.new(redis_store_class.redis_client_params) }
before do
- redis_client.call("flushdb")
+ redis_store_class.with(&:flushdb)
end
describe 'read and write' do
@@ -24,27 +23,30 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
# The response is 1001, so 4 bytes. Exercise counting an integer reply.
[[:set, 'foobar', 1000]] | [:incr, 'foobar'] | (4 + 6) | 4
- # Exercise counting empty multi bulk reply. Returns an empty hash `{}`
- [] | [:hgetall, 'foobar'] | (7 + 6) | 2
+ # Exercise counting empty multi bulk reply.
+ [] | [:hgetall, 'foobar'] | (7 + 6) | 0
# Hgetall response length is combined length of keys and values in the
# hash. Exercises counting of a multi bulk reply
- # Returns `{"field"=>"hello world"}`, 5 for field, 11 for hello world, 8 for {, }, 4 "s, =, >
- [[:hset, 'myhash', 'field', 'hello world']] | [:hgetall, 'myhash'] | (7 + 6) | (5 + 11 + 8)
+ [[:hset, 'myhash', 'field', 'hello world']] | [:hgetall, 'myhash'] | (7 + 6) | (5 + 11)
# Exercise counting of a bulk reply
[[:set, 'foo', 'bar' * 100]] | [:get, 'foo'] | (3 + 3) | (3 * 100)
- # Nested array response: [['foo', 0.0], ['bar', 1.0]]. Returns scores as float.
+ # Nested array response: [['foo', 0], ['bar', 1.1000000000000001]] due to Redis precision
+ # See https://github.com/redis/redis/issues/1499
[[:zadd, 'myset', 0, 'foo'],
- [:zadd, 'myset', 1, 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | (6 + 5 + 1 + 2 + 10) | (3 + 3 + 3 + 3)
+ [:zadd, 'myset', 1.1,
+ 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | (6 + 5 + 1 + 2 + 10) | (3 + 1 + 3 + 18)
end
with_them do
it 'counts bytes read and written' do
- setup.each { |cmd| redis_client.call(*cmd) }
- RequestStore.clear!
- redis_client.call(*command)
+ redis_store_class.with do |redis|
+ setup.each { |cmd| redis.call(cmd) }
+ RequestStore.clear!
+ redis.call(command)
+ end
expect(Gitlab::Instrumentation::Redis.read_bytes).to eq(expect_read)
expect(Gitlab::Instrumentation::Redis.write_bytes).to eq(expect_write)
@@ -58,35 +60,48 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
it 'counts successful requests' do
expect(instrumentation_class).to receive(:instance_count_request).with(1).and_call_original
- redis_client.call(:get, 'foobar')
+ redis_store_class.with { |redis| redis.call(:get, 'foobar') }
end
it 'counts successful pipelined requests' do
expect(instrumentation_class).to receive(:instance_count_request).with(2).and_call_original
expect(instrumentation_class).to receive(:instance_count_pipelined_request).with(2).and_call_original
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:get, '{foobar}baz')
+ end
end
end
context 'when encountering exceptions' do
- before do
- allow(redis_client.instance_variable_get(:@raw_connection)).to receive(:call).and_raise(
- RedisClient::Error)
+ where(:case_name, :exception, :exception_counter) do
+ 'generic exception' | Redis::CommandError.new | :instance_count_exception
+ 'moved redirection' | Redis::CommandError.new("MOVED 123 127.0.0.1:6380") | :instance_count_cluster_redirection
+ 'ask redirection' | Redis::CommandError.new("ASK 123 127.0.0.1:6380") | :instance_count_cluster_redirection
end
- it 'counts exception' do
- expect(instrumentation_class).to receive(:instance_count_exception)
- .with(instance_of(RedisClient::Error)).and_call_original
- expect(instrumentation_class).to receive(:log_exception)
- .with(instance_of(RedisClient::Error)).and_call_original
- expect(instrumentation_class).to receive(:instance_count_request).and_call_original
+ with_them do
+ before do
+ redis_store_class.with do |redis|
+ # We need to go 1 layer deeper to stub _client as we monkey-patch Redis::Client
+ # with the interceptor. Stubbing `redis` will skip the instrumentation_class.
+ allow(redis._client.instance_variable_get(:@raw_connection)).to receive(:call).and_raise(exception)
+ end
+ end
- expect do
- redis_client.call(:auth, 'foo', 'bar')
- end.to raise_error(RedisClient::Error)
+ it 'counts exception' do
+ expect(instrumentation_class).to receive(exception_counter)
+ .with(instance_of(Redis::CommandError)).and_call_original
+ expect(instrumentation_class).to receive(:log_exception)
+ .with(instance_of(Redis::CommandError)).and_call_original
+ expect(instrumentation_class).to receive(:instance_count_request).and_call_original
+
+ expect do
+ redis_store_class.with { |redis| redis.call(:auth, 'foo', 'bar') }
+ end.to raise_exception(Redis::CommandError)
+ end
end
end
@@ -99,7 +114,7 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
expect(instrumentation_class).to receive(:increment_cross_slot_request_count).and_call_original
expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
- redis_client.call(:mget, 'foo', 'bar')
+ redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
end
it 'does not count allowed cross-slot requests' do
@@ -107,7 +122,7 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
expect(instrumentation_class).to receive(:increment_allowed_cross_slot_request_count).and_call_original
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_client.call(:mget, 'foo', 'bar')
+ redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
end
end
@@ -116,7 +131,7 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_client.call(:mget, 'bar')
+ redis_store_class.with { |redis| redis.call(:get, 'bar') }
end
end
@@ -124,7 +139,7 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
- redis_client.call(:mget, '{foo}bar', '{foo}baz')
+ redis_store_class.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') }
end
end
@@ -135,7 +150,7 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
it 'still runs cross-slot validation' do
expect do
- redis_client.call('mget', 'foo', 'bar')
+ redis_store_class.with { |redis| redis.mget('foo', 'bar') }
end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
end
end
@@ -157,7 +172,7 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
expect(instrumentation_class).to receive(:instance_observe_duration).with(a_value > 0)
.and_call_original
- redis_client.call(*command)
+ redis_store_class.with { |redis| redis.call(*command) }
end
end
@@ -166,17 +181,21 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
expect(instrumentation_class).to receive(:instance_observe_duration).twice.with(a_value > 0)
.and_call_original
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:get, '{foobar}baz')
+ end
end
end
it 'raises error when keys are not from the same slot' do
expect do
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, 'foo')
- pipeline.call(:get, 'bar')
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, 'foo')
+ pipeline.call(:get, 'bar')
+ end
end
end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
end
@@ -200,11 +219,11 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
with_them do
it 'skips requests we do not want in the apdex' do
- setup.each { |cmd| redis_client.call(*cmd) }
+ setup.each { |cmd| redis_store_class.with { |redis| redis.call(*cmd) } }
expect(instrumentation_class).not_to receive(:instance_observe_duration)
- redis_client.call(*command)
+ redis_store_class.with { |redis| redis.call(*command) }
end
end
@@ -212,10 +231,12 @@ RSpec.describe Gitlab::Instrumentation::RedisClientMiddleware, :request_store, f
it 'skips requests that have blocking commands' do
expect(instrumentation_class).not_to receive(:instance_observe_duration)
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:rpush, '{foobar}baz', 1)
- pipeline.call(:brpop, '{foobar}baz', 0)
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:rpush, '{foobar}baz', 1)
+ pipeline.call(:brpop, '{foobar}baz', 0)
+ end
end
end
end
diff --git a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
deleted file mode 100644
index 2a160a9d316..00000000000
--- a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
+++ /dev/null
@@ -1,261 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-require 'rspec-parameterized'
-require 'support/helpers/rails_helpers'
-
-RSpec.describe Gitlab::Instrumentation::RedisInterceptor, :request_store, feature_category: :scalability do
- using RSpec::Parameterized::TableSyntax
- include RedisHelpers
-
- let_it_be(:redis_store_class) { define_helper_redis_store_class }
-
- before do
- redis_store_class.with(&:flushdb)
- end
-
- describe 'read and write' do
- where(:setup, :command, :expect_write, :expect_read) do
- # The response is 'OK', the request size is the combined size of array
- # elements. Exercise counting of a status reply.
- [] | [:set, 'foo', 'bar'] | 3 + 3 + 3 | 2
-
- # The response is 1001, so 4 bytes. Exercise counting an integer reply.
- [[:set, 'foobar', 1000]] | [:incr, 'foobar'] | 4 + 6 | 4
-
- # Exercise counting empty multi bulk reply
- [] | [:hgetall, 'foobar'] | 7 + 6 | 0
-
- # Hgetall response length is combined length of keys and values in the
- # hash. Exercises counting of a multi bulk reply
- [[:hset, 'myhash', 'field', 'hello world']] | [:hgetall, 'myhash'] | 7 + 6 | 5 + 11
-
- # Exercise counting of a bulk reply
- [[:set, 'foo', 'bar' * 100]] | [:get, 'foo'] | 3 + 3 | 3 * 100
-
- # Nested array response: [['foo', 0], ['bar', 1]]
- [[:zadd, 'myset', 0, 'foo'], [:zadd, 'myset', 1, 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | 6 + 5 + 1 + 2 + 10 | 3 + 1 + 3 + 1
- end
-
- with_them do
- it 'counts bytes read and written' do
- redis_store_class.with do |redis|
- setup.each { |cmd| redis.call(cmd) }
- RequestStore.clear!
- redis.call(command)
- end
-
- expect(Gitlab::Instrumentation::Redis.read_bytes).to eq(expect_read)
- expect(Gitlab::Instrumentation::Redis.write_bytes).to eq(expect_write)
- end
- end
- end
-
- describe 'counting' do
- let(:instrumentation_class) { redis_store_class.instrumentation_class }
-
- it 'counts successful requests' do
- expect(instrumentation_class).to receive(:instance_count_request).with(1).and_call_original
-
- redis_store_class.with { |redis| redis.call(:get, 'foobar') }
- end
-
- it 'counts successful pipelined requests' do
- expect(instrumentation_class).to receive(:instance_count_request).with(2).and_call_original
- expect(instrumentation_class).to receive(:instance_count_pipelined_request).with(2).and_call_original
-
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
- end
- end
- end
-
- context 'when encountering connection exceptions within process' do
- before do
- redis_store_class.with do |redis|
- allow(redis._client).to receive(:write).and_call_original
- end
- end
-
- it 'counts connection exceptions' do
- redis_store_class.with do |redis|
- expect(redis._client).to receive(:write).with([:get, 'foobar']).and_raise(::Redis::ConnectionError)
- end
-
- expect(instrumentation_class).to receive(:instance_count_connection_exception)
- .with(instance_of(Redis::ConnectionError)).and_call_original
-
- redis_store_class.with { |redis| redis.call(:get, 'foobar') }
- end
- end
-
- context 'when encountering exceptions' do
- where(:case_name, :exception, :exception_counter) do
- 'generic exception' | Redis::CommandError | :instance_count_exception
- 'moved redirection' | Redis::CommandError.new("MOVED 123 127.0.0.1:6380") | :instance_count_cluster_redirection
- 'ask redirection' | Redis::CommandError.new("ASK 123 127.0.0.1:6380") | :instance_count_cluster_redirection
- end
-
- with_them do
- before do
- redis_store_class.with do |redis|
- # We need to go 1 layer deeper to stub _client as we monkey-patch Redis::Client
- # with the interceptor. Stubbing `redis` will skip the instrumentation_class.
- allow(redis._client).to receive(:process).and_raise(exception)
- end
- end
-
- it 'counts exception' do
- expect(instrumentation_class).to receive(exception_counter)
- .with(instance_of(Redis::CommandError)).and_call_original
- expect(instrumentation_class).to receive(:log_exception)
- .with(instance_of(Redis::CommandError)).and_call_original
- expect(instrumentation_class).to receive(:instance_count_request).and_call_original
-
- expect do
- redis_store_class.with { |redis| redis.call(:auth, 'foo', 'bar') }
- end.to raise_exception(Redis::CommandError)
-
- expect(Thread.current[:redis_client_error_count]).to eq(0)
- end
- end
- end
-
- context 'in production environment' do
- before do
- stub_rails_env('production') # to avoid raising CrossSlotError
- end
-
- it 'counts disallowed cross-slot requests' do
- expect(instrumentation_class).to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
- end
-
- it 'does not count allowed cross-slot requests' do
- expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
- end
- end
-
- it 'does not count allowed non-cross-slot requests' do
- expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_store_class.with { |redis| redis.call(:get, 'bar') }
- end
- end
-
- it 'skips count for non-cross-slot requests' do
- expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- redis_store_class.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') }
- end
- end
-
- context 'without active RequestStore' do
- before do
- ::RequestStore.end!
- end
-
- it 'still runs cross-slot validation' do
- expect do
- redis_store_class.with { |redis| redis.mget('foo', 'bar') }
- end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
- end
- end
- end
-
- describe 'latency' do
- let(:instrumentation_class) { redis_store_class.instrumentation_class }
-
- describe 'commands in the apdex' do
- where(:command) do
- [
- [[:get, 'foobar']],
- [%w[GET foobar]]
- ]
- end
-
- with_them do
- it 'measures requests we want in the apdex' do
- expect(instrumentation_class).to receive(:instance_observe_duration).with(a_value > 0)
- .and_call_original
-
- redis_store_class.with { |redis| redis.call(*command) }
- end
- end
-
- context 'with pipelined commands' do
- it 'measures requests that do not have blocking commands' do
- expect(instrumentation_class).to receive(:instance_observe_duration).twice.with(a_value > 0)
- .and_call_original
-
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
- end
- end
- end
-
- it 'raises error when keys are not from the same slot' do
- expect do
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, 'foo')
- pipeline.call(:get, 'bar')
- end
- end
- end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
- end
- end
- end
-
- describe 'commands not in the apdex' do
- where(:setup, :command) do
- [['rpush', 'foobar', 1]] | ['brpop', 'foobar', 0]
- [['rpush', 'foobar', 1]] | ['blpop', 'foobar', 0]
- [['rpush', '{abc}foobar', 1]] | ['brpoplpush', '{abc}foobar', '{abc}bazqux', 0]
- [['rpush', '{abc}foobar', 1]] | ['brpoplpush', '{abc}foobar', '{abc}bazqux', 0]
- [['zadd', 'foobar', 1, 'a']] | ['bzpopmin', 'foobar', 0]
- [['zadd', 'foobar', 1, 'a']] | ['bzpopmax', 'foobar', 0]
- [['xadd', 'mystream', 1, 'myfield', 'mydata']] | ['xread', 'block', 1, 'streams', 'mystream', '0-0']
- [['xadd', 'foobar', 1, 'myfield', 'mydata'], ['xgroup', 'create', 'foobar', 'mygroup', 0]] | ['xreadgroup', 'group', 'mygroup', 'myconsumer', 'block', 1, 'streams', 'foobar', '0-0']
- [] | ['command']
- end
-
- with_them do
- it 'skips requests we do not want in the apdex' do
- redis_store_class.with { |redis| setup.each { |cmd| redis.call(*cmd) } }
-
- expect(instrumentation_class).not_to receive(:instance_observe_duration)
-
- redis_store_class.with { |redis| redis.call(*command) }
- end
- end
-
- context 'with pipelined commands' do
- it 'skips requests that have blocking commands' do
- expect(instrumentation_class).not_to receive(:instance_observe_duration)
-
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:rpush, '{foobar}baz', 1)
- pipeline.call(:brpop, '{foobar}baz', 0)
- end
- end
- end
- end
- end
- end
-end
diff --git a/spec/lib/gitlab/patch/node_loader_spec.rb b/spec/lib/gitlab/patch/node_loader_spec.rb
deleted file mode 100644
index 000083fc6d0..00000000000
--- a/spec/lib/gitlab/patch/node_loader_spec.rb
+++ /dev/null
@@ -1,80 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Patch::NodeLoader, feature_category: :redis do
- using RSpec::Parameterized::TableSyntax
-
- describe '#fetch_node_info' do
- let(:redis) { double(:redis) } # rubocop:disable RSpec/VerifiedDoubles
-
- # rubocop:disable Naming/InclusiveLanguage
- where(:case_name, :args, :value) do
- [
- [
- 'when only ip address is present',
- "07c37df 127.0.0.1:30004@31004 slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002 master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005 slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001 myself,master - 0 0 1 connected 0-5460",
- {
- '127.0.0.1:30004' => 'slave', '127.0.0.1:30002' => 'master', '127.0.0.1:30003' => 'master',
- '127.0.0.1:30005' => 'slave', '127.0.0.1:30006' => 'slave', '127.0.0.1:30001' => 'master'
- }
- ],
- [
- 'when hostname is present',
- "07c37df 127.0.0.1:30004@31004,host1 slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002,host2 master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003,host3 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005,host4 slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006,host5 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001,host6 myself,master - 0 0 1 connected 0-5460",
- {
- 'host1:30004' => 'slave', 'host2:30002' => 'master', 'host3:30003' => 'master',
- 'host4:30005' => 'slave', 'host5:30006' => 'slave', 'host6:30001' => 'master'
- }
- ],
- [
- 'when auxiliary fields are present',
- "07c37df 127.0.0.1:30004@31004,,shard-id=69bc slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002,,shard-id=114f master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003,,shard-id=fdb3 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005,,shard-id=114f slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006,,shard-id=fdb3 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001,,shard-id=69bc myself,master - 0 0 1 connected 0-5460",
- {
- '127.0.0.1:30004' => 'slave', '127.0.0.1:30002' => 'master', '127.0.0.1:30003' => 'master',
- '127.0.0.1:30005' => 'slave', '127.0.0.1:30006' => 'slave', '127.0.0.1:30001' => 'master'
- }
- ],
- [
- 'when hostname and auxiliary fields are present',
- "07c37df 127.0.0.1:30004@31004,host1,shard-id=69bc slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002,host2,shard-id=114f master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003,host3,shard-id=fdb3 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005,host4,shard-id=114f slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006,host5,shard-id=fdb3 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001,host6,shard-id=69bc myself,master - 0 0 1 connected 0-5460",
- {
- 'host1:30004' => 'slave', 'host2:30002' => 'master', 'host3:30003' => 'master',
- 'host4:30005' => 'slave', 'host5:30006' => 'slave', 'host6:30001' => 'master'
- }
- ]
- ]
- end
- # rubocop:enable Naming/InclusiveLanguage
-
- with_them do
- before do
- allow(redis).to receive(:call).with([:cluster, :nodes]).and_return(args)
- end
-
- it do
- expect(Redis::Cluster::NodeLoader.load_flags([redis])).to eq(value)
- end
- end
- end
-end
diff --git a/spec/lib/gitlab/patch/redis_cache_store_spec.rb b/spec/lib/gitlab/patch/redis_cache_store_spec.rb
index 21c256fdbbe..ae9c31e0c51 100644
--- a/spec/lib/gitlab/patch/redis_cache_store_spec.rb
+++ b/spec/lib/gitlab/patch/redis_cache_store_spec.rb
@@ -13,6 +13,8 @@ RSpec.describe Gitlab::Patch::RedisCacheStore, :use_clean_rails_redis_caching, f
cache.write('{user1}:x', 1)
cache.write('{user1}:y', 2)
cache.write('{user1}:z', 3)
+
+ cache.instance_variable_set(:@pipeline_batch_size, nil)
end
describe '#read_multi_mget' do
@@ -34,7 +36,7 @@ RSpec.describe Gitlab::Patch::RedisCacheStore, :use_clean_rails_redis_caching, f
end
context 'when reading large amount of keys' do
- let(:input_size) { 2000 }
+ let(:input_size) { 2100 }
let(:chunk_size) { 1000 }
shared_examples 'read large amount of keys' do
@@ -45,10 +47,11 @@ RSpec.describe Gitlab::Patch::RedisCacheStore, :use_clean_rails_redis_caching, f
::Gitlab::Redis::ClusterUtil.cluster?(redis.default_store)
if normal_cluster || multistore_cluster
- expect_next_instances_of(Gitlab::Redis::CrossSlot::Pipeline, 2) do |pipeline|
- obj = instance_double(::Redis)
- expect(pipeline).to receive(:pipelined).and_yield(obj)
- expect(obj).to receive(:get).exactly(chunk_size).times
+ times = (input_size.to_f / chunk_size).ceil
+ expect(redis).to receive(:pipelined).exactly(times).times.and_call_original
+
+ expect_next_instances_of(::Redis::PipelinedConnection, times) do |p|
+ expect(p).to receive(:get).at_most(chunk_size).times
end
else
expect(redis).to receive(:mget).and_call_original
diff --git a/spec/lib/gitlab/patch/redis_client_spec.rb b/spec/lib/gitlab/patch/redis_client_spec.rb
index af094e9e0d2..3e2d46dd19c 100644
--- a/spec/lib/gitlab/patch/redis_client_spec.rb
+++ b/spec/lib/gitlab/patch/redis_client_spec.rb
@@ -6,7 +6,7 @@ RSpec.describe Gitlab::Patch::RedisClient, feature_category: :redis do
include RedisHelpers
let_it_be(:redis_store_class) { define_helper_redis_store_class }
- let_it_be(:redis_client) { RedisClient.new(redis_store_class.redis_client_params) }
+ let_it_be(:redis_client) { RedisClient.new(redis_store_class.params) }
before do
Thread.current[:redis_client_error_count] = 1
diff --git a/spec/lib/gitlab/patch/redis_store_factory_spec.rb b/spec/lib/gitlab/patch/redis_store_factory_spec.rb
new file mode 100644
index 00000000000..be37a044ac1
--- /dev/null
+++ b/spec/lib/gitlab/patch/redis_store_factory_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Patch::RedisStoreFactory, feature_category: :redis do
+ describe '#create' do
+ let(:params) { { host: 'localhost' } }
+
+ subject(:factory_create) { ::Redis::Store::Factory.create(params) } # rubocop:disable Rails/SaveBang -- redis-store does not implement create!
+
+ context 'when using standalone Redis' do
+ it 'does not create ClusterStore' do
+ expect(Gitlab::Redis::ClusterStore).not_to receive(:new)
+
+ factory_create
+ end
+ end
+
+ context 'when using a Redis Cluster' do
+ let(:params) { { nodes: ["redis://localhost:6001", "redis://localhost:6002"] } }
+
+ it 'creates a ClusterStore' do
+ expect(Gitlab::Redis::ClusterStore).to receive(:new).with(params.merge({ raw: false }))
+
+ factory_create
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/rack_attack/store_spec.rb b/spec/lib/gitlab/rack_attack/store_spec.rb
index 19b3f239d91..efe6f9382f9 100644
--- a/spec/lib/gitlab/rack_attack/store_spec.rb
+++ b/spec/lib/gitlab/rack_attack/store_spec.rb
@@ -102,7 +102,7 @@ RSpec.describe Gitlab::RackAttack::Store, :clean_gitlab_redis_rate_limiting, fea
before do
broken_redis = Redis.new(
url: 'redis://127.0.0.0:0',
- instrumentation_class: Gitlab::Redis::RateLimiting.instrumentation_class
+ custom: { instrumentation_class: Gitlab::Redis::RateLimiting.instrumentation_class }
)
allow(Gitlab::Redis::RateLimiting).to receive(:with).and_yield(broken_redis)
end
diff --git a/spec/lib/gitlab/redis/cluster_store_spec.rb b/spec/lib/gitlab/redis/cluster_store_spec.rb
new file mode 100644
index 00000000000..aaeee6156a0
--- /dev/null
+++ b/spec/lib/gitlab/redis/cluster_store_spec.rb
@@ -0,0 +1,116 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# This spec only runs if a Redis Cluster is configured for Gitlab::Redis::Cache.
+# ::Redis::Cluster fetches the cluster details from the server on `initialize` and will raise
+# an error if the cluster is not found.
+#
+# An example would be the following in config/redis.yml assuming gdk is set up with redis-cluster.
+# test:
+# cache
+# cluster:
+# - "redis://127.0.0.1:6003"
+# - "redis://127.0.0.1:6004"
+# - "redis://127.0.0.1:6005"
+RSpec.describe Gitlab::Redis::ClusterStore, :clean_gitlab_redis_cache,
+ feature_category: :redis, if: ::Gitlab::Redis::Cache.params[:nodes] do
+ let(:params) { ::Gitlab::Redis::Cache.params }
+
+ subject(:store) { ::Redis::Store::Factory.create(params) } # rubocop:disable Rails/SaveBang -- not a rails method
+
+ describe '.new' do
+ it 'initialises a cluster store' do
+ expect(store).to be_instance_of(::Gitlab::Redis::ClusterStore)
+ end
+
+ it 'extends Serialization by default' do
+ expect(store.is_a?(::Redis::Store::Serialization)).to eq(true)
+ end
+
+ it 'sets a default serializer when left empty' do
+ expect(store.instance_variable_get(:@serializer)).to eq(Marshal)
+ end
+
+ context 'when serializer field is defined' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(serializer: Class) }
+
+ it 'sets serializer according to the options' do
+ expect(store.instance_variable_get(:@serializer)).to eq(Class)
+ end
+ end
+
+ context 'when marshalling field is defined' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(marshalling: true, serializer: Class) }
+
+ it 'overrides serializer with Marshal' do
+ expect(store.instance_variable_get(:@serializer)).to eq(Marshal)
+ end
+ end
+
+ context 'when marshalling field is false' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(marshalling: false, serializer: Class) }
+
+ it 'overrides serializer with Marshal' do
+ expect(store.instance_variable_get(:@serializer)).to eq(nil)
+ end
+ end
+
+ context 'when namespace is defined' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(namespace: 'testing') }
+
+ it 'extends namespace' do
+ expect(store.is_a?(::Redis::Store::Namespace)).to eq(true)
+ end
+
+ it 'write keys with namespace' do
+ store.set('testkey', 1)
+
+ ::Gitlab::Redis::Cache.with do |conn|
+ expect(conn.exists('testing:testkey')).to eq(1)
+ end
+ end
+ end
+ end
+
+ describe '#set' do
+ context 'when ttl is added' do
+ it 'writes the key and sets a ttl' do
+ expect(store.set('test', 1, expire_after: 100)).to eq('OK')
+
+ expect(store.ttl('test')).to be > 95
+ expect(store.get('test')).to eq(1)
+ end
+ end
+
+ context 'when there is no ttl' do
+ it 'sets the key' do
+ expect(store.set('test', 1)).to eq('OK')
+
+ expect(store.get('test')).to eq(1)
+ expect(store.ttl('test')).to eq(-1)
+ end
+ end
+ end
+
+ describe '#setnx' do
+ context 'when ttl is added' do
+ it 'writes the key if not exists and sets a ttl' do
+ expect(store.setnx('test', 1, expire_after: 100)).to eq([true, true])
+ expect(store.ttl('test')).to be > 95
+ expect(store.get('test')).to eq(1)
+ expect(store.setnx('test', 1, expire_after: 100)).to eq([false, true])
+ end
+ end
+
+ context 'when there is no ttl' do
+ it 'writes the key if not exists' do
+ expect(store.setnx('test', 1)).to eq(true)
+ expect(store.setnx('test', 1)).to eq(false)
+
+ expect(store.get('test')).to eq(1)
+ expect(store.ttl('test')).to eq(-1)
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/redis/cluster_util_spec.rb b/spec/lib/gitlab/redis/cluster_util_spec.rb
index a5175450145..1cd6a450584 100644
--- a/spec/lib/gitlab/redis/cluster_util_spec.rb
+++ b/spec/lib/gitlab/redis/cluster_util_spec.rb
@@ -5,10 +5,14 @@ require 'spec_helper'
RSpec.describe Gitlab::Redis::ClusterUtil, feature_category: :scalability do
using RSpec::Parameterized::TableSyntax
+ let(:router_stub) { instance_double(::RedisClient::Cluster::Router) }
+
+ before do
+ allow(::RedisClient::Cluster::Router).to receive(:new).and_return(router_stub)
+ end
+
describe '.cluster?' do
context 'when MultiStore' do
- let(:redis_cluster) { instance_double(::Redis::Cluster) }
-
where(:pri_store, :sec_store, :expected_val) do
:cluster | :cluster | true
:cluster | :single | true
@@ -17,10 +21,7 @@ RSpec.describe Gitlab::Redis::ClusterUtil, feature_category: :scalability do
end
before do
- # stub all initialiser steps in Redis::Cluster.new to avoid connecting to a Redis Cluster node
- allow(::Redis::Cluster).to receive(:new).and_return(redis_cluster)
- allow(redis_cluster).to receive(:is_a?).with(::Redis::Cluster).and_return(true)
- allow(redis_cluster).to receive(:id).and_return(1)
+ allow(router_stub).to receive(:node_keys).and_return([])
allow(Gitlab::Redis::MultiStore).to receive(:same_redis_store?).and_return(false)
skip_default_enabled_yaml_check
@@ -28,8 +29,8 @@ RSpec.describe Gitlab::Redis::ClusterUtil, feature_category: :scalability do
with_them do
it 'returns expected value' do
- primary_redis = pri_store == :cluster ? ::Redis.new(cluster: ['redis://localhost:6000']) : ::Redis.new
- secondary_redis = sec_store == :cluster ? ::Redis.new(cluster: ['redis://localhost:6000']) : ::Redis.new
+ primary_redis = pri_store == :cluster ? Redis::Cluster.new(nodes: ['redis://localhost:6000']) : Redis.new
+ secondary_redis = sec_store == :cluster ? Redis::Cluster.new(nodes: ['redis://localhost:6000']) : Redis.new
primary_pool = ConnectionPool.new { primary_redis }
secondary_pool = ConnectionPool.new { secondary_redis }
multistore = Gitlab::Redis::MultiStore.new(primary_pool, secondary_pool, 'teststore')
@@ -48,16 +49,8 @@ RSpec.describe Gitlab::Redis::ClusterUtil, feature_category: :scalability do
end
context 'when is Redis::Cluster' do
- let(:redis_cluster) { instance_double(::Redis::Cluster) }
-
- before do
- # stub all initialiser steps in Redis::Cluster.new to avoid connecting to a Redis Cluster node
- allow(::Redis::Cluster).to receive(:new).and_return(redis_cluster)
- allow(redis_cluster).to receive(:is_a?).with(::Redis::Cluster).and_return(true)
- end
-
it 'returns true' do
- expect(described_class.cluster?(::Redis.new(cluster: ['redis://localhost:6000']))).to be_truthy
+ expect(described_class.cluster?(Redis::Cluster.new(nodes: ['redis://localhost:6000']))).to be_truthy
end
end
end
diff --git a/spec/lib/gitlab/redis/command_builder_spec.rb b/spec/lib/gitlab/redis/command_builder_spec.rb
new file mode 100644
index 00000000000..7ebb0228d2d
--- /dev/null
+++ b/spec/lib/gitlab/redis/command_builder_spec.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# references specs in https://github.com/redis-rb/redis-client/blob/master/test/redis_client/command_builder_test.rb
+# we add `handles nil arguments` to test our own added logic
+RSpec.describe Gitlab::Redis::CommandBuilder, feature_category: :redis do
+ describe '.generate' do
+ def call(*args, **kwargs)
+ described_class.generate(args, kwargs)
+ end
+
+ it 'handles nil arguments' do
+ expect(call("a", nil)).to eq(["a", ""])
+ end
+
+ it 'handles positional arguments' do
+ expect(call("a", "b", "c")).to eq(%w[a b c])
+ end
+
+ it 'handles arrays' do
+ expect(call("a", %w[b c])).to eq(%w[a b c])
+ end
+
+ it 'handles hashes' do
+ expect(call("a", { "b" => "c" })).to eq(%w[a b c])
+ end
+
+ it 'handles symbols' do
+ expect(call(:a, { b: :c }, :d)).to eq(%w[a b c d])
+ end
+
+ it 'handles numerics' do
+ expect(call(1, 2.3)).to eq(["1", "2.3"])
+ end
+
+ it 'handles kwargs booleans' do
+ expect(call(ttl: nil, ex: false, withscores: true)).to eq(["withscores"])
+ end
+
+ it 'handles kwargs values' do
+ expect(call(ttl: 42)).to eq(%w[ttl 42])
+ end
+
+ it 'handles nil kwargs' do
+ expect(call(%i[a b c])).to eq(%w[a b c])
+ end
+
+ it 'raises error on unsupported types' do
+ expect { call(hash: {}) }.to raise_error(TypeError)
+ end
+
+ it 'raises error on empty commands' do
+ expect { call }.to raise_error(ArgumentError)
+ end
+ end
+end
diff --git a/spec/lib/gitlab/redis/cross_slot_spec.rb b/spec/lib/gitlab/redis/cross_slot_spec.rb
deleted file mode 100644
index 4e9830f4110..00000000000
--- a/spec/lib/gitlab/redis/cross_slot_spec.rb
+++ /dev/null
@@ -1,134 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Redis::CrossSlot, feature_category: :redis do
- include RedisHelpers
-
- let_it_be(:redis_store_class) { define_helper_redis_store_class }
-
- before do
- redis_store_class.with(&:flushdb)
- end
-
- describe '.pipelined' do
- context 'when using redis client' do
- before do
- redis_store_class.with { |redis| redis.set('a', 1) }
- end
-
- it 'performs redis-rb pipelined' do
- expect(Gitlab::Redis::CrossSlot::Router).not_to receive(:new)
-
- expect(
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_store_class.with do |redis|
- described_class::Pipeline.new(redis).pipelined do |p|
- p.get('a')
- p.set('b', 1)
- end
- end
- end
- ).to eq(%w[1 OK])
- end
- end
-
- context 'when using with MultiStore' do
- let_it_be(:primary_db) { 1 }
- let_it_be(:secondary_db) { 2 }
- let_it_be(:primary_store) { create_redis_store(redis_store_class.params, db: primary_db, serializer: nil) }
- let_it_be(:secondary_store) { create_redis_store(redis_store_class.params, db: secondary_db, serializer: nil) }
- let_it_be(:primary_pool) { ConnectionPool.new { primary_store } }
- let_it_be(:secondary_pool) { ConnectionPool.new { secondary_store } }
- let_it_be(:multistore) { Gitlab::Redis::MultiStore.new(primary_pool, secondary_pool, 'testing') }
-
- before do
- primary_store.set('a', 1)
- secondary_store.set('a', 1)
- skip_default_enabled_yaml_check
- end
-
- it 'performs multistore pipelined' do
- expect(Gitlab::Redis::CrossSlot::Router).not_to receive(:new)
-
- expect(
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- multistore.with_borrowed_connection do
- described_class::Pipeline.new(multistore).pipelined do |p|
- p.get('a')
- p.set('b', 1)
- end
- end
- end
- ).to eq(%w[1 OK])
- end
- end
-
- context 'when using Redis::Cluster' do
- # Only stub redis client internals since the CI pipeline does not run a Redis Cluster
- let(:redis) { double(:redis) } # rubocop:disable RSpec/VerifiedDoubles
- let(:client) { double(:client) } # rubocop:disable RSpec/VerifiedDoubles
- let(:pipeline) { double(:pipeline) } # rubocop:disable RSpec/VerifiedDoubles
-
- let(:arguments) { %w[a b c d] }
-
- subject do
- described_class::Pipeline.new(redis).pipelined do |p|
- arguments.each { |key| p.get(key) }
- end
- end
-
- before do
- allow(redis).to receive(:_client).and_return(client)
- allow(redis).to receive(:pipelined).and_yield(pipeline)
- allow(client).to receive(:instance_of?).with(::Redis::Cluster).and_return(true)
- end
-
- it 'fan-out and fan-in commands to separate shards' do
- # simulate fan-out to 3 shards with random order
- expect(client).to receive(:_find_node_key).exactly(4).times.and_return(3, 2, 1, 3)
-
- arguments.each do |key|
- f = double('future') # rubocop:disable RSpec/VerifiedDoubles
- expect(pipeline).to receive(:get).with(key).and_return(f)
- expect(f).to receive(:value).and_return(key)
- end
-
- expect(subject).to eq(arguments)
- end
-
- shared_examples 'fallback on cross-slot' do |redirection|
- context 'when redis cluster undergoing slot migration' do
- before do
- allow(pipeline).to receive(:get).and_raise(::Redis::CommandError.new("#{redirection} 1 127.0.0.1:7001"))
- end
-
- it 'logs error and executes sequentially' do
- expect(client).to receive(:_find_node_key).exactly(4).times.and_return(3, 2, 1, 3)
- expect(Gitlab::ErrorTracking).to receive(:log_exception).with(an_instance_of(::Redis::CommandError))
-
- arguments.each do |key|
- expect(redis).to receive(:get).with(key).and_return(key)
- end
-
- subject
- end
- end
- end
-
- it_behaves_like 'fallback on cross-slot', 'MOVED'
- it_behaves_like 'fallback on cross-slot', 'ASK'
-
- context 'when receiving non-MOVED/ASK command errors' do
- before do
- allow(pipeline).to receive(:get).and_raise(::Redis::CommandError.new)
- allow(client).to receive(:_find_node_key).exactly(4).times.and_return(3, 2, 1, 3)
- end
-
- it 'raises error' do
- expect { subject }.to raise_error(::Redis::CommandError)
- end
- end
- end
- end
-end
diff --git a/spec/lib/gitlab/redis/multi_store_spec.rb b/spec/lib/gitlab/redis/multi_store_spec.rb
index 348215d553c..0d841be4e3c 100644
--- a/spec/lib/gitlab/redis/multi_store_spec.rb
+++ b/spec/lib/gitlab/redis/multi_store_spec.rb
@@ -58,6 +58,7 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do
context 'when primary_store is not a ::Redis instance' do
before do
allow(primary_store).to receive(:is_a?).with(::Redis).and_return(false)
+ allow(primary_store).to receive(:is_a?).with(::Redis::Cluster).and_return(false)
end
it 'fails with exception' do
@@ -69,6 +70,7 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do
context 'when secondary_store is not a ::Redis instance' do
before do
allow(secondary_store).to receive(:is_a?).with(::Redis).and_return(false)
+ allow(secondary_store).to receive(:is_a?).with(::Redis::Cluster).and_return(false)
end
it 'fails with exception' do
@@ -618,35 +620,6 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do
end
end
- context 'when either store is a an instance of ::Redis::Cluster' do
- let(:pipeline) { double }
- let(:client) { double }
-
- before do
- allow(client).to receive(:instance_of?).with(::Redis::Cluster).and_return(true)
- allow(pipeline).to receive(:pipelined)
- multi_store.with_borrowed_connection do
- allow(multi_store.default_store).to receive(:_client).and_return(client)
- end
- end
-
- it 'calls cross-slot pipeline within multistore' do
- if name == :pipelined
- # we intentionally exclude `.and_call_original` since primary_store/secondary_store
- # may not be running on a proper Redis Cluster.
- multi_store.with_borrowed_connection do
- expect(Gitlab::Redis::CrossSlot::Pipeline).to receive(:new)
- .with(multi_store.default_store)
- .exactly(:once)
- .and_return(pipeline)
- expect(Gitlab::Redis::CrossSlot::Pipeline).not_to receive(:new).with(multi_store.non_default_store)
- end
- end
-
- subject
- end
- end
-
context 'when with_readonly_pipeline is used' do
it 'calls the default store only' do
expect(primary_store).to receive(:send).and_call_original
diff --git a/spec/lib/gitlab/redis/sessions_spec.rb b/spec/lib/gitlab/redis/sessions_spec.rb
index 874822e3e6a..e822b7399b7 100644
--- a/spec/lib/gitlab/redis/sessions_spec.rb
+++ b/spec/lib/gitlab/redis/sessions_spec.rb
@@ -8,9 +8,9 @@ RSpec.describe Gitlab::Redis::Sessions do
describe '#store' do
subject(:store) { described_class.store(namespace: described_class::SESSION_NAMESPACE) }
- # Check that Gitlab::Redis::Sessions is configured as RedisStore.
+ # Check that Gitlab::Redis::Sessions is configured as RedisStore or ClusterStore
it 'instantiates an instance of Redis::Store' do
- expect(store).to be_instance_of(::Redis::Store)
+ expect([::Redis::Store, ::Gitlab::Redis::ClusterStore].include?(store.class)).to eq(true)
end
end
end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index c3e9f5a178e..a4303a6e0fc 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -544,18 +544,18 @@ end
Rack::Test::UploadedFile.prepend(TouchRackUploadedFile)
-# Monkey-patch to enable ActiveSupport::Notifications for Redis commands
+# Inject middleware to enable ActiveSupport::Notifications for Redis commands
module RedisCommands
module Instrumentation
- def process(commands, &block)
- ActiveSupport::Notifications.instrument('redis.process_commands', commands: commands) do
- super(commands, &block)
+ def call(command, redis_config)
+ ActiveSupport::Notifications.instrument('redis.process_commands', commands: command) do
+ super(command, redis_config)
end
end
end
end
-Redis::Client.prepend(RedisCommands::Instrumentation)
+RedisClient.register(RedisCommands::Instrumentation)
module UsersInternalAllowExclusiveLease
extend ActiveSupport::Concern
diff --git a/spec/support/helpers/dns_helpers.rb b/spec/support/helpers/dns_helpers.rb
index 0250e432609..27310c7e04c 100644
--- a/spec/support/helpers/dns_helpers.rb
+++ b/spec/support/helpers/dns_helpers.rb
@@ -60,8 +60,8 @@ module DnsHelpers
def permit_redis!
# https://github.com/redis-rb/redis-client/blob/v0.11.2/lib/redis_client/ruby_connection.rb#L51 uses Socket.tcp that
# calls Addrinfo.getaddrinfo internally.
- hosts = Gitlab::Redis::ALL_CLASSES.map do |redis_instance|
- redis_instance.redis_client_params[:host]
+ hosts = Gitlab::Redis::ALL_CLASSES.flat_map do |redis_instance|
+ redis_instance.params[:host] || redis_instance.params[:nodes]&.map { |n| n[:host] }
end.uniq.compact
hosts.each do |host|
diff --git a/spec/support/matchers/exceed_redis_call_limit.rb b/spec/support/matchers/exceed_redis_call_limit.rb
index 2b1e1ebad23..2cb92089723 100644
--- a/spec/support/matchers/exceed_redis_call_limit.rb
+++ b/spec/support/matchers/exceed_redis_call_limit.rb
@@ -14,7 +14,7 @@ module ExceedRedisCallLimitHelpers
end
def verify_commands_count(command, expected, block)
- @actual = build_recorder(block).by_command(command).count
+ @actual = build_recorder(block).by_command(command.to_s).count
@actual > expected
end
diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml
index 8791220f11e..7e113122eac 100644
--- a/spec/support/rspec_order_todo.yml
+++ b/spec/support/rspec_order_todo.yml
@@ -6292,7 +6292,6 @@
- './spec/lib/gitlab/instrumentation/rate_limiting_gates_spec.rb'
- './spec/lib/gitlab/instrumentation/redis_base_spec.rb'
- './spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb'
-- './spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb'
- './spec/lib/gitlab/instrumentation/redis_spec.rb'
- './spec/lib/gitlab/internal_post_receive/response_spec.rb'
- './spec/lib/gitlab/issuable/clone/attributes_rewriter_spec.rb'
diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_import/object_import_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_import/object_import_shared_examples.rb
index 3dbe43d822f..55ce520caab 100644
--- a/spec/support/shared_examples/lib/gitlab/bitbucket_import/object_import_shared_examples.rb
+++ b/spec/support/shared_examples/lib/gitlab/bitbucket_import/object_import_shared_examples.rb
@@ -25,7 +25,7 @@ RSpec.shared_examples Gitlab::BitbucketImport::ObjectImporter do
expect(Gitlab::JobWaiter).to receive(:notify).with(waiter_key, anything)
- worker.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(project_id, {}, waiter_key)
end
end
@@ -49,7 +49,7 @@ RSpec.shared_examples Gitlab::BitbucketImport::ObjectImporter do
expect(Gitlab::BitbucketImport::Logger).to receive(:info).twice
expect_next(worker.importer_class, project, kind_of(Hash)).to receive(:execute)
- worker.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(project_id, {}, waiter_key)
end
it_behaves_like 'notifies the waiter'
@@ -62,7 +62,7 @@ RSpec.shared_examples Gitlab::BitbucketImport::ObjectImporter do
it 'tracks the error' do
expect(Gitlab::Import::ImportFailureService).to receive(:track).once
- worker.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(project_id, {}, waiter_key)
end
end
@@ -74,7 +74,7 @@ RSpec.shared_examples Gitlab::BitbucketImport::ObjectImporter do
it 'tracks the error and raises the error' do
expect(Gitlab::Import::ImportFailureService).to receive(:track).once
- expect { worker.perform(project_id, {}, waiter_key) }.to raise_error(StandardError)
+ expect { worker.class.perform_inline(project_id, {}, waiter_key) }.to raise_error(StandardError)
end
end
end
@@ -85,7 +85,7 @@ RSpec.shared_examples Gitlab::BitbucketImport::ObjectImporter do
it 'does not call the importer' do
expect_next(worker.importer_class).not_to receive(:execute)
- worker.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(project_id, {}, waiter_key)
end
it_behaves_like 'notifies the waiter'
diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb
index 45248f57683..89eec531b87 100644
--- a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb
+++ b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb
@@ -19,6 +19,10 @@ RSpec.shared_examples Gitlab::BitbucketServerImport::ObjectImporter do
let(:project_id) { project_id }
let(:waiter_key) { 'key' }
+ before do
+ allow(Gitlab::JobWaiter).to receive(:notify).with(waiter_key, anything, ttl: Gitlab::Import::JOB_WAITER_TTL)
+ end
+
shared_examples 'notifies the waiter' do
specify do
allow_next(worker.importer_class).to receive(:execute)
diff --git a/spec/support/shared_examples/redis/redis_shared_examples.rb b/spec/support/shared_examples/redis/redis_shared_examples.rb
index 1c153b7c31b..2f387528740 100644
--- a/spec/support/shared_examples/redis/redis_shared_examples.rb
+++ b/spec/support/shared_examples/redis/redis_shared_examples.rb
@@ -80,18 +80,17 @@ RSpec.shared_examples "redis_shared_examples" do
context 'with new format' do
it_behaves_like 'redis store' do
- let(:config_file_name) { config_new_format_host }
+ # use new format host without sentinel details as `.to_s` checks `config` which
+ # tries to resolve master/replica details with an actual sentinel instance.
+ # https://github.com/redis-rb/redis-client/blob/v0.18.0/lib/redis_client/sentinel_config.rb#L128
+ let(:config_file_name) { "spec/fixtures/config/redis_new_format_host_standalone.yml" }
let(:host) { "development-host:#{redis_port}" }
end
end
end
- describe '.redis_client_params' do
- # .redis_client_params wraps over `.redis_store_options` by modifying its outputs
- # to be compatible with `RedisClient`. We test for compatibility in this block while
- # the contents of redis_store_options are tested in the `.params` block.
-
- subject { described_class.new(rails_env).redis_client_params }
+ describe '.params' do
+ subject { described_class.new(rails_env).params }
let(:rails_env) { 'development' }
let(:config_file_name) { config_old_format_socket }
@@ -103,56 +102,6 @@ RSpec.shared_examples "redis_shared_examples" do
end
end
- context 'when url is host based' do
- context 'with old format' do
- let(:config_file_name) { config_old_format_host }
-
- it 'does not raise ArgumentError for invalid keywords' do
- expect { RedisClient.config(**subject) }.not_to raise_error
- end
-
- it_behaves_like 'instrumentation_class in custom key'
- end
-
- context 'with new format' do
- let(:config_file_name) { config_new_format_host }
-
- where(:rails_env, :host) do
- [
- %w[development development-host],
- %w[test test-host],
- %w[production production-host]
- ]
- end
-
- with_them do
- it 'does not raise ArgumentError for invalid keywords in SentinelConfig' do
- expect(subject[:name]).to eq(host)
- expect { RedisClient.sentinel(**subject) }.not_to raise_error
- end
-
- it_behaves_like 'instrumentation_class in custom key'
- end
- end
- end
-
- context 'when url contains unix socket reference' do
- let(:config_file_name) { config_old_format_socket }
-
- it 'does not raise ArgumentError for invalid keywords' do
- expect { RedisClient.config(**subject) }.not_to raise_error
- end
-
- it_behaves_like 'instrumentation_class in custom key'
- end
- end
-
- describe '.params' do
- subject { described_class.new(rails_env).params }
-
- let(:rails_env) { 'development' }
- let(:config_file_name) { config_old_format_socket }
-
it 'withstands mutation' do
params1 = described_class.params
params2 = described_class.params
@@ -251,10 +200,16 @@ RSpec.shared_examples "redis_shared_examples" do
with_them do
it 'returns hash with host, port, db, and password' do
- is_expected.to include(host: host, password: 'mynewpassword', port: redis_port, db: redis_database)
+ is_expected.to include(name: host, password: 'mynewpassword', db: redis_database)
is_expected.not_to have_key(:url)
end
+
+ it 'does not raise ArgumentError for invalid keywords in SentinelConfig' do
+ expect { RedisClient.sentinel(**subject) }.not_to raise_error
+ end
end
+
+ it_behaves_like 'instrumentation_class in custom key'
end
context 'with redis cluster format' do
@@ -272,13 +227,19 @@ RSpec.shared_examples "redis_shared_examples" do
it 'returns hash with cluster and password' do
is_expected.to include(
password: 'myclusterpassword',
- cluster: [
+ nodes: [
{ host: "#{host}1", port: redis_port },
{ host: "#{host}2", port: redis_port }
]
)
is_expected.not_to have_key(:url)
end
+
+ it 'does not raise ArgumentError for invalid keywords in ClusterConfig' do
+ expect { RedisClient::ClusterConfig.new(**subject) }.not_to raise_error
+ end
+
+ it_behaves_like 'instrumentation_class in custom key'
end
end
end
diff --git a/spec/support_specs/helpers/redis_commands/recorder_spec.rb b/spec/support_specs/helpers/redis_commands/recorder_spec.rb
index ef46db5e29e..5c200f15bae 100644
--- a/spec/support_specs/helpers/redis_commands/recorder_spec.rb
+++ b/spec/support_specs/helpers/redis_commands/recorder_spec.rb
@@ -13,7 +13,7 @@ RSpec.describe RedisCommands::Recorder, :use_clean_rails_redis_caching do
it 'records Redis commands' do
recorder = described_class.new { cache.read('key1') }
- expect(recorder.log).to include([:get, 'cache:gitlab:key1'])
+ expect(recorder.log).to include(['get', 'cache:gitlab:key1'])
end
end
@@ -35,10 +35,10 @@ RSpec.describe RedisCommands::Recorder, :use_clean_rails_redis_caching do
cache.delete('key1')
end
- expect(recorder.log).to include([:set, 'cache:gitlab:key1', anything, anything, anything])
- expect(recorder.log).to include([:get, 'cache:gitlab:key1'])
- expect(recorder.log).to include([:get, 'cache:gitlab:key2'])
- expect(recorder.log).to include([:del, 'cache:gitlab:key1'])
+ expect(recorder.log).to include(['set', 'cache:gitlab:key1', anything, anything, anything])
+ expect(recorder.log).to include(['get', 'cache:gitlab:key1'])
+ expect(recorder.log).to include(['get', 'cache:gitlab:key2'])
+ expect(recorder.log).to include(['del', 'cache:gitlab:key1'])
end
it 'does not record commands before the call' do
@@ -48,8 +48,8 @@ RSpec.describe RedisCommands::Recorder, :use_clean_rails_redis_caching do
cache.read('key1')
end
- expect(recorder.log).not_to include([:set, anything, anything])
- expect(recorder.log).to include([:get, 'cache:gitlab:key1'])
+ expect(recorder.log).not_to include(['set', anything, anything])
+ expect(recorder.log).to include(['get', 'cache:gitlab:key1'])
end
it 'refreshes recording after reinitialization' do
@@ -68,15 +68,15 @@ RSpec.describe RedisCommands::Recorder, :use_clean_rails_redis_caching do
cache.read('key4')
end
- expect(recorder1.log).to include([:get, 'cache:gitlab:key2'])
- expect(recorder1.log).not_to include([:get, 'cache:gitlab:key1'])
- expect(recorder1.log).not_to include([:get, 'cache:gitlab:key3'])
- expect(recorder1.log).not_to include([:get, 'cache:gitlab:key4'])
+ expect(recorder1.log).to include(['get', 'cache:gitlab:key2'])
+ expect(recorder1.log).not_to include(['get', 'cache:gitlab:key1'])
+ expect(recorder1.log).not_to include(['get', 'cache:gitlab:key3'])
+ expect(recorder1.log).not_to include(['get', 'cache:gitlab:key4'])
- expect(recorder2.log).to include([:get, 'cache:gitlab:key4'])
- expect(recorder2.log).not_to include([:get, 'cache:gitlab:key1'])
- expect(recorder2.log).not_to include([:get, 'cache:gitlab:key2'])
- expect(recorder2.log).not_to include([:get, 'cache:gitlab:key3'])
+ expect(recorder2.log).to include(['get', 'cache:gitlab:key4'])
+ expect(recorder2.log).not_to include(['get', 'cache:gitlab:key1'])
+ expect(recorder2.log).not_to include(['get', 'cache:gitlab:key2'])
+ expect(recorder2.log).not_to include(['get', 'cache:gitlab:key3'])
end
end
@@ -91,10 +91,10 @@ RSpec.describe RedisCommands::Recorder, :use_clean_rails_redis_caching do
cache.delete('key2')
end
- expect(recorder.log).to include([:set, 'cache:gitlab:key1', anything, anything, anything])
- expect(recorder.log).to include([:get, 'cache:gitlab:key1'])
- expect(recorder.log).not_to include([:get, 'cache:gitlab:key2'])
- expect(recorder.log).not_to include([:del, 'cache:gitlab:key2'])
+ expect(recorder.log).to include(['set', 'cache:gitlab:key1', anything, anything, anything])
+ expect(recorder.log).to include(['get', 'cache:gitlab:key1'])
+ expect(recorder.log).not_to include(['get', 'cache:gitlab:key2'])
+ expect(recorder.log).not_to include(['del', 'cache:gitlab:key2'])
end
end
@@ -107,7 +107,7 @@ RSpec.describe RedisCommands::Recorder, :use_clean_rails_redis_caching do
cache.delete('key2')
end
- expect(recorder.by_command(:del)).to match_array([[:del, 'cache:gitlab:key2']])
+ expect(recorder.by_command('del')).to match_array([['del', 'cache:gitlab:key2']])
end
end
diff --git a/spec/workers/gitlab/github_gists_import/import_gist_worker_spec.rb b/spec/workers/gitlab/github_gists_import/import_gist_worker_spec.rb
index d11b044b093..4cb0a25f892 100644
--- a/spec/workers/gitlab/github_gists_import/import_gist_worker_spec.rb
+++ b/spec/workers/gitlab/github_gists_import/import_gist_worker_spec.rb
@@ -108,17 +108,24 @@ RSpec.describe Gitlab::GithubGistsImport::ImportGistWorker, feature_category: :i
before do
allow(importer).to receive(:execute).and_return(importer_result)
+ allow(Gitlab::GithubGistsImport::Representation::Gist)
+ .to receive(:from_json_hash)
+ .with(anything)
+ .and_return(gist_object)
end
it 'tracks and logs error' do
+ # use `anything` since jid is created in Sidekiq's middleware. `jid` does not exist until
+ # perform_inline is called.
expect(Gitlab::GithubImport::Logger)
.to receive(:error)
- .with(log_attributes.merge('message' => 'importer failed', 'exception.message' => 'error_message'))
+ .with(log_attributes.merge('message' => 'importer failed', 'exception.message' => 'error_message',
+ 'jid' => anything))
expect(Gitlab::JobWaiter)
.to receive(:notify)
- .with('some_key', subject.jid, ttl: Gitlab::Import::JOB_WAITER_TTL)
+ .with('some_key', anything, ttl: Gitlab::Import::JOB_WAITER_TTL)
- subject.perform(user.id, gist_hash, 'some_key')
+ subject.class.perform_inline(user.id, gist_hash, 'some_key') # perform_inline calls .perform
expect_snowplow_event(
category: 'Gitlab::GithubGistsImport::ImportGistWorker',
@@ -130,7 +137,7 @@ RSpec.describe Gitlab::GithubGistsImport::ImportGistWorker, feature_category: :i
end
it 'persists failure' do
- expect { subject.perform(user.id, gist_hash, 'some_key') }
+ expect { subject.class.perform_inline(user.id, gist_hash, 'some_key') }
.to change { ImportFailure.where(user: user).count }.from(0).to(1)
expect(ImportFailure.where(user_id: user.id).first).to have_attributes(
diff --git a/spec/workers/gitlab/jira_import/import_issue_worker_spec.rb b/spec/workers/gitlab/jira_import/import_issue_worker_spec.rb
index 3b2114ee488..c9c4c6e2d2b 100644
--- a/spec/workers/gitlab/jira_import/import_issue_worker_spec.rb
+++ b/spec/workers/gitlab/jira_import/import_issue_worker_spec.rb
@@ -46,7 +46,7 @@ RSpec.describe Gitlab::JiraImport::ImportIssueWorker, feature_category: :importe
context 'when import label does not exist' do
it 'does not record import failure' do
- subject.perform(project.id, 123, issue_attrs, some_key)
+ subject.class.perform_inline(project.id, 123, issue_attrs, some_key)
expect(label.issues.count).to eq(0)
expect(Gitlab::Cache::Import::Caching.read(Gitlab::JiraImport.failed_issues_counter_cache_key(project.id)).to_i).to eq(0)
@@ -57,7 +57,7 @@ RSpec.describe Gitlab::JiraImport::ImportIssueWorker, feature_category: :importe
before do
Gitlab::JiraImport.cache_import_label_id(project.id, label.id)
- subject.perform(project.id, 123, issue_attrs, some_key)
+ subject.class.perform_inline(project.id, 123, issue_attrs, some_key)
end
it 'does not record import failure' do
diff --git a/spec/workers/redis_migration_worker_spec.rb b/spec/workers/redis_migration_worker_spec.rb
index 9f29c84a948..d8f2cacdb13 100644
--- a/spec/workers/redis_migration_worker_spec.rb
+++ b/spec/workers/redis_migration_worker_spec.rb
@@ -30,7 +30,7 @@ RSpec.describe RedisMigrationWorker, :clean_gitlab_redis_shared_state, feature_c
end
def redis
- ::Redis.new(::Gitlab::Redis::Cache.params)
+ ::Gitlab::Redis::Cache.redis
end
end
end