From a7f569b2e9a87c17d373b4914db00e8b859518e7 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Wed, 14 Feb 2024 16:52:28 +0000
Subject: [PATCH] Revert Redis v5 gem upgrade
This reverts merge request !144300
---
.rubocop_todo/layout/line_length.yml | 1 +
.../lint/ambiguous_operator_precedence.yml | 1 +
.rubocop_todo/rspec/feature_category.yml | 1 +
.rubocop_todo/rspec/named_subject.yml | 1 +
.../style/inline_disable_annotation.yml | 5 +
Gemfile | 5 +-
Gemfile.checksum | 4 +-
Gemfile.lock | 11 +-
app/models/active_session.rb | 2 +-
.../concerns/limited_capacity/job_tracker.rb | 4 +-
config/initializers/7_redis.rb | 6 +
config/initializers/action_cable.rb | 2 +-
config/initializers/peek.rb | 2 +-
config/initializers/session_store.rb | 4 +-
config/initializers/sidekiq.rb | 8 +-
doc/administration/redis/troubleshooting.md | 2 +-
doc/development/redis.md | 4 +-
doc/development/redis/new_redis_instance.md | 4 +-
.../elastic/indexing_control_service.rb | 2 +-
.../backfill_project_pipeline_status_ttl.rb | 4 +-
lib/gitlab/cache/import/caching.rb | 4 +-
lib/gitlab/diff/highlight_cache.rb | 4 +-
.../discussions_diff/highlight_cache.rb | 6 +-
lib/gitlab/etag_caching/store.rb | 2 +-
lib/gitlab/exclusive_lease.rb | 3 +-
.../redis_cluster_validator.rb | 4 +-
.../instrumentation/redis_interceptor.rb | 55 ++++
lib/gitlab/issues/rebalancing/state.rb | 2 +-
lib/gitlab/markdown_cache/redis/store.rb | 2 +-
lib/gitlab/patch/command_loader.rb | 19 ++
lib/gitlab/patch/node_loader.rb | 52 ++++
lib/gitlab/patch/redis_cache_store.rb | 4 +-
lib/gitlab/patch/redis_cluster.rb | 21 ++
lib/gitlab/patch/redis_store_factory.rb | 16 --
lib/gitlab/patch/slot_loader.rb | 19 ++
lib/gitlab/redis/cluster_store.rb | 81 ------
lib/gitlab/redis/cluster_util.rb | 6 +-
lib/gitlab/redis/command_builder.rb | 53 ----
lib/gitlab/redis/cross_slot.rb | 141 ++++++++++
lib/gitlab/redis/multi_store.rb | 12 +-
lib/gitlab/redis/wrapper.rb | 37 +--
lib/gitlab/repository_hash_cache.rb | 2 -
lib/gitlab/set_cache.rb | 2 +-
.../duplicate_jobs/duplicate_job.rb | 2 +-
.../product_analytics_tracking_spec.rb | 3 +-
.../redis_new_format_host_standalone.yml | 8 -
...le_subscription_adapter_identifier_spec.rb | 2 +-
spec/initializers/session_store_spec.rb | 2 +-
.../migration_support/exclusive_lock_spec.rb | 4 +-
...ckfill_project_pipeline_status_ttl_spec.rb | 2 +-
.../runner_backoff/communicator_spec.rb | 2 +-
.../discussions_diff/highlight_cache_spec.rb | 2 +-
.../external_authorization/cache_spec.rb | 4 +-
.../redis_client_middleware_spec.rb | 111 +++-----
.../instrumentation/redis_interceptor_spec.rb | 261 ++++++++++++++++++
spec/lib/gitlab/patch/node_loader_spec.rb | 80 ++++++
.../gitlab/patch/redis_cache_store_spec.rb | 13 +-
spec/lib/gitlab/patch/redis_client_spec.rb | 2 +-
.../gitlab/patch/redis_store_factory_spec.rb | 29 --
spec/lib/gitlab/rack_attack/store_spec.rb | 2 +-
spec/lib/gitlab/redis/cluster_store_spec.rb | 116 --------
spec/lib/gitlab/redis/cluster_util_spec.rb | 27 +-
spec/lib/gitlab/redis/command_builder_spec.rb | 57 ----
spec/lib/gitlab/redis/cross_slot_spec.rb | 134 +++++++++
spec/lib/gitlab/redis/multi_store_spec.rb | 31 ++-
spec/lib/gitlab/redis/sessions_spec.rb | 4 +-
spec/spec_helper.rb | 10 +-
spec/support/helpers/dns_helpers.rb | 4 +-
.../matchers/exceed_redis_call_limit.rb | 2 +-
spec/support/rspec_order_todo.yml | 1 +
.../object_import_shared_examples.rb | 10 +-
.../object_import_shared_examples.rb | 4 -
.../redis/redis_shared_examples.rb | 79 ++++--
.../helpers/redis_commands/recorder_spec.rb | 40 +--
.../import_gist_worker_spec.rb | 15 +-
.../jira_import/import_issue_worker_spec.rb | 4 +-
spec/workers/redis_migration_worker_spec.rb | 2 +-
77 files changed, 1071 insertions(+), 618 deletions(-)
create mode 100644 lib/gitlab/instrumentation/redis_interceptor.rb
create mode 100644 lib/gitlab/patch/command_loader.rb
create mode 100644 lib/gitlab/patch/node_loader.rb
create mode 100644 lib/gitlab/patch/redis_cluster.rb
delete mode 100644 lib/gitlab/patch/redis_store_factory.rb
create mode 100644 lib/gitlab/patch/slot_loader.rb
delete mode 100644 lib/gitlab/redis/cluster_store.rb
delete mode 100644 lib/gitlab/redis/command_builder.rb
create mode 100644 lib/gitlab/redis/cross_slot.rb
delete mode 100644 spec/fixtures/config/redis_new_format_host_standalone.yml
create mode 100644 spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
create mode 100644 spec/lib/gitlab/patch/node_loader_spec.rb
delete mode 100644 spec/lib/gitlab/patch/redis_store_factory_spec.rb
delete mode 100644 spec/lib/gitlab/redis/cluster_store_spec.rb
delete mode 100644 spec/lib/gitlab/redis/command_builder_spec.rb
create mode 100644 spec/lib/gitlab/redis/cross_slot_spec.rb
diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml
index 754d31a99b2dc..2095d2c1ca3e1 100644
--- a/.rubocop_todo/layout/line_length.yml
+++ b/.rubocop_todo/layout/line_length.yml
@@ -3703,6 +3703,7 @@ Layout/LineLength:
- 'spec/lib/gitlab/import_export/uploads_manager_spec.rb'
- 'spec/lib/gitlab/import_export/version_checker_spec.rb'
- 'spec/lib/gitlab/import_sources_spec.rb'
+ - 'spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb'
- 'spec/lib/gitlab/issuable_metadata_spec.rb'
- 'spec/lib/gitlab/issues/rebalancing/state_spec.rb'
- 'spec/lib/gitlab/jira/dvcs_spec.rb'
diff --git a/.rubocop_todo/lint/ambiguous_operator_precedence.yml b/.rubocop_todo/lint/ambiguous_operator_precedence.yml
index a7cac8214c08c..5b352842a870d 100644
--- a/.rubocop_todo/lint/ambiguous_operator_precedence.yml
+++ b/.rubocop_todo/lint/ambiguous_operator_precedence.yml
@@ -102,6 +102,7 @@ Lint/AmbiguousOperatorPrecedence:
- 'spec/lib/gitlab/database/batch_count_spec.rb'
- 'spec/lib/gitlab/database/consistency_checker_spec.rb'
- 'spec/lib/gitlab/graphql/tracers/metrics_tracer_spec.rb'
+ - 'spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb'
- 'spec/lib/gitlab/issues/rebalancing/state_spec.rb'
- 'spec/lib/gitlab/kroki_spec.rb'
- 'spec/lib/gitlab/memory/instrumentation_spec.rb'
diff --git a/.rubocop_todo/rspec/feature_category.yml b/.rubocop_todo/rspec/feature_category.yml
index c13df3a295bd0..bfb96e1a6a1aa 100644
--- a/.rubocop_todo/rspec/feature_category.yml
+++ b/.rubocop_todo/rspec/feature_category.yml
@@ -3586,6 +3586,7 @@ RSpec/FeatureCategory:
- '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/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml
index 5520a5210f983..6d447453985c6 100644
--- a/.rubocop_todo/rspec/named_subject.yml
+++ b/.rubocop_todo/rspec/named_subject.yml
@@ -2331,6 +2331,7 @@ RSpec/NamedSubject:
- 'spec/lib/gitlab/rack_attack_spec.rb'
- 'spec/lib/gitlab/reactive_cache_set_cache_spec.rb'
- 'spec/lib/gitlab/redis/boolean_spec.rb'
+ - 'spec/lib/gitlab/redis/cross_slot_spec.rb'
- 'spec/lib/gitlab/redis/db_load_balancing_spec.rb'
- 'spec/lib/gitlab/redis/multi_store_spec.rb'
- 'spec/lib/gitlab/redis/queues_spec.rb'
diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml
index 31f59f51bf7c6..4bf891cd5c33c 100644
--- a/.rubocop_todo/style/inline_disable_annotation.yml
+++ b/.rubocop_todo/style/inline_disable_annotation.yml
@@ -2517,6 +2517,7 @@ Style/InlineDisableAnnotation:
- 'lib/gitlab/import_export/project/relation_factory.rb'
- 'lib/gitlab/import_sources.rb'
- 'lib/gitlab/instrumentation/redis_cluster_validator.rb'
+ - 'lib/gitlab/instrumentation/redis_interceptor.rb'
- 'lib/gitlab/internal_events.rb'
- 'lib/gitlab/issuable/clone/copy_resource_events_service.rb'
- 'lib/gitlab/issues/rebalancing/state.rb'
@@ -2555,6 +2556,7 @@ Style/InlineDisableAnnotation:
- 'lib/gitlab/pagination/offset_pagination.rb'
- 'lib/gitlab/pagination_delegate.rb'
- 'lib/gitlab/patch/action_cable_subscription_adapter_identifier.rb'
+ - 'lib/gitlab/patch/node_loader.rb'
- 'lib/gitlab/patch/prependable.rb'
- 'lib/gitlab/patch/redis_cache_store.rb'
- 'lib/gitlab/patch/sidekiq_cron_poller.rb'
@@ -2571,6 +2573,7 @@ Style/InlineDisableAnnotation:
- 'lib/gitlab/rack_attack.rb'
- 'lib/gitlab/rack_attack/request.rb'
- 'lib/gitlab/rack_attack/store.rb'
+ - 'lib/gitlab/redis/cross_slot.rb'
- 'lib/gitlab/redis/hll.rb'
- 'lib/gitlab/redis/multi_store.rb'
- 'lib/gitlab/reference_extractor.rb'
@@ -2939,7 +2942,9 @@ Style/InlineDisableAnnotation:
- 'spec/lib/gitlab/pagination/keyset/order_spec.rb'
- 'spec/lib/gitlab/pagination/keyset/simple_order_builder_spec.rb'
- 'spec/lib/gitlab/patch/database_config_spec.rb'
+ - 'spec/lib/gitlab/patch/node_loader_spec.rb'
- 'spec/lib/gitlab/quick_actions/dsl_spec.rb'
+ - 'spec/lib/gitlab/redis/cross_slot_spec.rb'
- 'spec/lib/gitlab/redis/multi_store_spec.rb'
- 'spec/lib/gitlab/search/abuse_detection_spec.rb'
- 'spec/lib/gitlab/shard_health_cache_spec.rb'
diff --git a/Gemfile b/Gemfile
index 527424e45f3f3..f2acfd06e73a7 100644
--- a/Gemfile
+++ b/Gemfile
@@ -288,9 +288,8 @@ gem 'js_regex', '~> 3.8' # rubocop:todo Gemfile/MissingFeatureCategory
gem 'device_detector' # rubocop:todo Gemfile/MissingFeatureCategory
# Redis
-gem 'redis-namespace', '~> 1.10.0', feature_category: :redis
-gem 'redis', '~> 5.0.0', feature_category: :redis
-gem 'redis-clustering', '~> 5.0.0', feature_category: :redis
+gem 'redis', '~> 4.8.0' # rubocop:todo Gemfile/MissingFeatureCategory
+gem 'redis-namespace', '~> 1.10.0' # rubocop:todo Gemfile/MissingFeatureCategory
gem 'connection_pool', '~> 2.4' # rubocop:todo Gemfile/MissingFeatureCategory
# Redis session store
diff --git a/Gemfile.checksum b/Gemfile.checksum
index f7daa2a8e9d3d..6c0918adaec2c 100644
--- a/Gemfile.checksum
+++ b/Gemfile.checksum
@@ -523,11 +523,9 @@
{"name":"recaptcha","version":"5.12.3","platform":"ruby","checksum":"37d1894add9e70a54d0c6c7f0ecbeedffbfa7d075acfbd4c509818dfdebdb7ee"},
{"name":"recursive-open-struct","version":"1.1.3","platform":"ruby","checksum":"a3538a72552fcebcd0ada657bdff313641a4a5fbc482c08cfb9a65acb1c9de5a"},
{"name":"redcarpet","version":"3.6.0","platform":"ruby","checksum":"8ad1889c0355ff4c47174af14edd06d62f45a326da1da6e8a121d59bdcd2e9e9"},
-{"name":"redis","version":"5.0.8","platform":"ruby","checksum":"3b770ea597850b26d6a9718fa184241e53e6c8a7ae0486ee8bfaefd29f26f3d8"},
+{"name":"redis","version":"4.8.0","platform":"ruby","checksum":"2000cf5014669c9dc821704b6d322a35a9a33852a95208911d9175d63b448a44"},
{"name":"redis-actionpack","version":"5.4.0","platform":"ruby","checksum":"f10cf649ab05914716d63334d7f709221ecc883b87cf348f90ecfe0c35ea3540"},
{"name":"redis-client","version":"0.20.0","platform":"ruby","checksum":"239ac38fe4f0b62c8d2d8641989319b736b7dd40eebec868fd874a14686bdc8c"},
-{"name":"redis-cluster-client","version":"0.7.5","platform":"ruby","checksum":"12fd1c9eda17157a5cd2ce46afba13a024c28d24922092299a8daa9f46e4e78a"},
-{"name":"redis-clustering","version":"5.0.8","platform":"ruby","checksum":"8e2f3de3b1a700668eeac59125636e01be6ecd985e635a4d5649c47d71f6e166"},
{"name":"redis-namespace","version":"1.10.0","platform":"ruby","checksum":"2c1c6ea7c6c5e343e75b9bee3aa4c265e364a5b9966507397467af2bb3758d94"},
{"name":"redis-rack","version":"3.0.0","platform":"ruby","checksum":"abb50b82ae10ad4d11ca2e4901bfc2b98256cdafbbd95f80c86fc9e001478380"},
{"name":"redis-store","version":"1.10.0","platform":"ruby","checksum":"f258894f9f7e82834308a3d86242294f0cff2c9db9ae66e5cb4c553a5ec8b09e"},
diff --git a/Gemfile.lock b/Gemfile.lock
index 5e8c7f544bb16..fcc61ec94abff 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1385,19 +1385,13 @@ GEM
json
recursive-open-struct (1.1.3)
redcarpet (3.6.0)
- redis (5.0.8)
- redis-client (>= 0.17.0)
+ redis (4.8.0)
redis-actionpack (5.4.0)
actionpack (>= 5, < 8)
redis-rack (>= 2.1.0, < 4)
redis-store (>= 1.1.0, < 2)
redis-client (0.20.0)
connection_pool
- redis-cluster-client (0.7.5)
- redis-client (~> 0.12)
- redis-clustering (5.0.8)
- redis (= 5.0.8)
- redis-cluster-client (>= 0.7.0)
redis-namespace (1.10.0)
redis (>= 4)
redis-rack (3.0.0)
@@ -2075,9 +2069,8 @@ DEPENDENCIES
rbtrace (~> 0.4)
re2 (= 2.7.0)
recaptcha (~> 5.12)
- redis (~> 5.0.0)
+ redis (~> 4.8.0)
redis-actionpack (~> 5.4.0)
- redis-clustering (~> 5.0.0)
redis-namespace (~> 1.10.0)
request_store (~> 1.5.1)
responders (~> 3.0)
diff --git a/app/models/active_session.rb b/app/models/active_session.rb
index 756e0b7fbf9cd..2eb9c9bca7f24 100644
--- a/app/models/active_session.rb
+++ b/app/models/active_session.rb
@@ -84,7 +84,7 @@ def self.set(user, request)
)
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
pipeline.setex(
key_name(user.id, session_private_id),
expiry,
diff --git a/app/workers/concerns/limited_capacity/job_tracker.rb b/app/workers/concerns/limited_capacity/job_tracker.rb
index 2ac3c5e991bfb..b4d884f914df0 100644
--- a/app/workers/concerns/limited_capacity/job_tracker.rb
+++ b/app/workers/concerns/limited_capacity/job_tracker.rb
@@ -21,7 +21,7 @@ def initialize(namespace)
def register(jid, max_jids)
with_redis do |redis|
- redis.eval(LUA_REGISTER_SCRIPT, keys: [counter_key], argv: [jid.to_s, max_jids.to_i])
+ redis.eval(LUA_REGISTER_SCRIPT, keys: [counter_key], argv: [jid, max_jids])
end.present?
end
@@ -59,7 +59,7 @@ def counter_key
end
def remove_job_keys(redis, keys)
- redis.srem?(counter_key, keys) if keys.present?
+ redis.srem?(counter_key, keys)
end
def with_redis(&block)
diff --git a/config/initializers/7_redis.rb b/config/initializers/7_redis.rb
index c7385612602a0..fbd2cbaabad64 100644
--- a/config/initializers/7_redis.rb
+++ b/config/initializers/7_redis.rb
@@ -21,6 +21,12 @@
# :nocov:
# rubocop:enable Gitlab/NoCodeCoverageComment
+Redis::Client.prepend(Gitlab::Instrumentation::RedisInterceptor)
+Redis::Cluster::NodeLoader.prepend(Gitlab::Patch::NodeLoader)
+Redis::Cluster::SlotLoader.prepend(Gitlab::Patch::SlotLoader)
+Redis::Cluster::CommandLoader.prepend(Gitlab::Patch::CommandLoader)
+Redis::Cluster.prepend(Gitlab::Patch::RedisCluster)
+
# this only instruments `RedisClient` used in `Sidekiq.redis`
RedisClient.register(Gitlab::Instrumentation::RedisClientMiddleware)
RedisClient.prepend(Gitlab::Patch::RedisClient)
diff --git a/config/initializers/action_cable.rb b/config/initializers/action_cable.rb
index 93fb80b38fa57..dc69a108f56e5 100644
--- a/config/initializers/action_cable.rb
+++ b/config/initializers/action_cable.rb
@@ -25,7 +25,7 @@
# https://github.com/rails/rails/blob/bb5ac1623e8de08c1b7b62b1368758f0d3bb6379/actioncable/lib/action_cable/subscription_adapter/redis.rb#L18
ActionCable::SubscriptionAdapter::Redis.redis_connector = lambda do |config|
args = config.except(:adapter, :channel_prefix)
- .merge(custom: { instrumentation_class: "ActionCable" })
+ .merge(instrumentation_class: ::Gitlab::Instrumentation::Redis::ActionCable)
::Redis.new(args)
end
diff --git a/config/initializers/peek.rb b/config/initializers/peek.rb
index 72e7d3f10f98b..e1c59851fb1a9 100644
--- a/config/initializers/peek.rb
+++ b/config/initializers/peek.rb
@@ -6,7 +6,7 @@
Peek.singleton_class.prepend ::Gitlab::PerformanceBar::WithTopLevelWarnings
Rails.application.config.peek.adapter = :redis, {
- client: Gitlab::Redis::Cache.redis,
+ client: ::Redis.new(Gitlab::Redis::Cache.params),
expires_in: 5.minutes
}
diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb
index 70bd3d012d096..7f410d7bf7bbd 100644
--- a/config/initializers/session_store.rb
+++ b/config/initializers/session_store.rb
@@ -29,12 +29,12 @@
"_gitlab_session"
end
-::Redis::Store::Factory.prepend(Gitlab::Patch::RedisStoreFactory)
+store = Gitlab::Redis::Sessions.store(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE)
Rails.application.configure do
config.session_store(
:redis_store, # Using the cookie_store would enable session replay attacks.
- redis_server: Gitlab::Redis::Sessions.params.merge(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE),
+ redis_store: store,
key: cookie_key,
secure: Gitlab.config.gitlab.https,
httponly: true,
diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb
index d7798a5ccd74c..926972a9d41e2 100644
--- a/config/initializers/sidekiq.rb
+++ b/config/initializers/sidekiq.rb
@@ -16,13 +16,7 @@ def load_cron_jobs!
end
# Custom Queues configuration
-#
-# We omit :command_builder since Sidekiq::RedisConnection performs a deep clone using
-# Marshal.load(Marshal.dump(options.slice(*keys))) on the Redis config and Gitlab::Redis::CommandBuilder
-# can't be referred to.
-#
-# We do not need the custom command builder since Sidekiq will handle the typing of Redis arguments.
-queues_config_hash = Gitlab::Redis::Queues.params.except(:command_builder)
+queues_config_hash = Gitlab::Redis::Queues.redis_client_params
enable_json_logs = Gitlab.config.sidekiq.log_format != 'text'
diff --git a/doc/administration/redis/troubleshooting.md b/doc/administration/redis/troubleshooting.md
index 7ec6414bf45ed..aebb300422356 100644
--- a/doc/administration/redis/troubleshooting.md
+++ b/doc/administration/redis/troubleshooting.md
@@ -124,7 +124,7 @@ To make sure your configuration is correct:
1. Run in the console:
```ruby
- redis = Gitlab::Redis::SharedState.redis
+ redis = Redis.new(Gitlab::Redis::SharedState.params)
redis.info
```
diff --git a/doc/development/redis.md b/doc/development/redis.md
index 368f670936792..f9afe5e071890 100644
--- a/doc/development/redis.md
+++ b/doc/development/redis.md
@@ -81,10 +81,10 @@ Developers are highly encouraged to use [hash-tags](https://redis.io/docs/refere
where appropriate to facilitate future adoption of Redis Cluster in more Redis types. For example, the Namespace model uses hash-tags
for its [config cache keys](https://gitlab.com/gitlab-org/gitlab/-/blob/1a12337058f260d38405886d82da5e8bb5d8da0b/app/models/namespace.rb#L786).
-To perform multi-key commands, developers may use the [`.pipelined`](https://github.com/redis-rb/redis-cluster-client#interfaces) method which splits and sends commands to each node and aggregates replies.
+To perform multi-key commands, developers may use the [`Gitlab::Redis::CrossSlot::Pipeline`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/redis/cross_slot.rb) wrapper.
However, this does not work for [transactions](https://redis.io/docs/interact/transactions/) as Redis Cluster does not support cross-slot transactions.
-For `Rails.cache`, we handle the `MGET` command found in `read_multi_get` by [patching it](https://gitlab.com/gitlab-org/gitlab/-/blob/c2bad2aac25e2f2778897bd4759506a72b118b15/lib/gitlab/patch/redis_cache_store.rb#L10) to use the `.pipelined` method.
+For `Rails.cache`, we handle the `MGET` command found in `read_multi_get` by [patching it](https://gitlab.com/gitlab-org/gitlab/-/blob/c2bad2aac25e2f2778897bd4759506a72b118b15/lib/gitlab/patch/redis_cache_store.rb#L10) to use the `Gitlab::Redis::CrossSlot::Pipeline` wrapper.
The minimum size of the pipeline is set to 1000 commands and it can be adjusted by using the `GITLAB_REDIS_CLUSTER_PIPELINE_BATCH_LIMIT` environment variable.
## Redis in structured logging
diff --git a/doc/development/redis/new_redis_instance.md b/doc/development/redis/new_redis_instance.md
index 054364fce5e44..ff5394cef8f22 100644
--- a/doc/development/redis/new_redis_instance.md
+++ b/doc/development/redis/new_redis_instance.md
@@ -147,8 +147,8 @@ module Gitlab
# Don't use multistore if redis.foo configuration is not provided
return super if config_fallback?
- primary_store = init_redis(params)
- secondary_store = init_redis(config_fallback.params)
+ primary_store = ::Redis.new(params)
+ secondary_store = ::Redis.new(config_fallback.params)
MultiStore.new(primary_store, secondary_store, store_name)
end
diff --git a/ee/app/services/elastic/indexing_control_service.rb b/ee/app/services/elastic/indexing_control_service.rb
index 1b82c2f3a76a8..50ececd471062 100644
--- a/ee/app/services/elastic/indexing_control_service.rb
+++ b/ee/app/services/elastic/indexing_control_service.rb
@@ -68,7 +68,7 @@ def resume_processing!
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if queue_size == 0
- redis.pipelined do |p|
+ Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |p|
p.del(redis_score_key)
p.del(redis_set_key)
end
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 7886abdb6d408..2672498b6271f 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 @@ def perform(keys)
ttl_jitter = 2.hours.to_i
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
keys.each { |key| pipeline.expire(key, ttl_duration + rand(-ttl_jitter..ttl_jitter)) }
end
end
@@ -25,7 +25,7 @@ def scan_match_pattern
end
def redis
- @redis ||= Gitlab::Redis::Cache.redis
+ @redis ||= ::Redis.new(Gitlab::Redis::Cache.params)
end
end
end
diff --git a/lib/gitlab/cache/import/caching.rb b/lib/gitlab/cache/import/caching.rb
index 0b24137076dd6..c538e5162c7b3 100644
--- a/lib/gitlab/cache/import/caching.rb
+++ b/lib/gitlab/cache/import/caching.rb
@@ -139,7 +139,7 @@ def self.set_includes?(raw_key, value)
key = cache_key_for(raw_key)
with_redis do |redis|
- redis.sismember(key, value || value.to_s)
+ redis.sismember(key, value)
end
end
@@ -162,7 +162,7 @@ def self.values_from_set(raw_key)
def self.write_multiple(mapping, key_prefix: nil, timeout: TIMEOUT)
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(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 0d5a66175f8e5..dc5f4e1b32432 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -197,9 +197,7 @@ def read_cache
record_hit_ratio(results)
results.map! do |result|
- unless result.nil?
- Gitlab::Json.parse(gzip_decompress(result.force_encoding(Encoding::UTF_8)), symbolize_names: true)
- end
+ Gitlab::Json.parse(gzip_decompress(result), symbolize_names: true) unless result.nil?
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 dbdc8c3c95c7c..18ff7c28e174b 100644
--- a/lib/gitlab/discussions_diff/highlight_cache.rb
+++ b/lib/gitlab/discussions_diff/highlight_cache.rb
@@ -16,7 +16,7 @@ class << self
def write_multiple(mapping)
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis.pipelined do |pipelined|
+ Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipelined|
mapping.each do |raw_key, value|
key = cache_key_for(raw_key)
@@ -42,7 +42,7 @@ def read_multiple(raw_keys)
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if Gitlab::Redis::ClusterUtil.cluster?(redis)
- redis.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
keys.each { |key| pipeline.get(key) }
end
else
@@ -54,7 +54,7 @@ def read_multiple(raw_keys)
content.map! do |lines|
next unless lines
- Gitlab::Json.parse(gzip_decompress(lines.force_encoding(Encoding::UTF_8))).map! do |line|
+ Gitlab::Json.parse(gzip_decompress(lines)).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 57c160c9607e7..5fdf5ac943627 100644
--- a/lib/gitlab/etag_caching/store.rb
+++ b/lib/gitlab/etag_caching/store.rb
@@ -17,7 +17,7 @@ def touch(*keys, only_if_missing: false)
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
with_redis do |redis|
- redis.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(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 b596956077454..d470fb503cbe2 100644
--- a/lib/gitlab/exclusive_lease.rb
+++ b/lib/gitlab/exclusive_lease.rb
@@ -62,7 +62,6 @@ def self.throttle(key, group: nil, period: 1.hour, count: 1, &block)
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])
@@ -143,7 +142,7 @@ def waiting?
# 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.to_i])
+ result = redis.eval(LUA_RENEW_SCRIPT, keys: [@redis_shared_state_key], argv: [@uuid, @timeout])
result == @uuid
end
end
diff --git a/lib/gitlab/instrumentation/redis_cluster_validator.rb b/lib/gitlab/instrumentation/redis_cluster_validator.rb
index 5c3655f8f803f..948132e6edddb 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-clustering'
+require 'redis'
module Gitlab
module Instrumentation
@@ -230,7 +230,7 @@ def has_cross_slot_keys?(keys)
end
def key_slot(key)
- ::RedisClient::Cluster::KeySlotConverter.convert(extract_hash_tag(key))
+ ::Redis::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
new file mode 100644
index 0000000000000..9c89af6a0dc58
--- /dev/null
+++ b/lib/gitlab/instrumentation/redis_interceptor.rb
@@ -0,0 +1,55 @@
+# 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 12cc5f6e5ddf4..c60dac6f571e5 100644
--- a/lib/gitlab/issues/rebalancing/state.rb
+++ b/lib/gitlab/issues/rebalancing/state.rb
@@ -100,7 +100,7 @@ def remove_current_project_id_cache
def refresh_keys_expiration
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(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 85aeaba92c4a5..af9098c3300de 100644
--- a/lib/gitlab/markdown_cache/redis/store.rb
+++ b/lib/gitlab/markdown_cache/redis/store.rb
@@ -11,7 +11,7 @@ def self.bulk_read(subjects)
data = Gitlab::Redis::Cache.with do |r|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- r.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(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
new file mode 100644
index 0000000000000..357b6270b0dd5
--- /dev/null
+++ b/lib/gitlab/patch/command_loader.rb
@@ -0,0 +1,19 @@
+# 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
new file mode 100644
index 0000000000000..85237abc1376b
--- /dev/null
+++ b/lib/gitlab/patch/node_loader.rb
@@ -0,0 +1,52 @@
+# 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: <ip:port@cport[,hostname[,auxiliary_field=value]*]>
+ 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 35c617ba72b1c..96729056ce5f7 100644
--- a/lib/gitlab/patch/redis_cache_store.rb
+++ b/lib/gitlab/patch/redis_cache_store.rb
@@ -20,7 +20,7 @@ def delete_multi_entries(entries, **options)
delete_count = 0
redis.with do |conn|
entries.each_slice(pipeline_batch_size) do |subset|
- delete_count += conn.pipelined do |pipeline|
+ delete_count += Gitlab::Redis::CrossSlot::Pipeline.new(conn).pipelined do |pipeline|
subset.each { |entry| pipeline.del(entry) }
end.sum
end
@@ -58,7 +58,7 @@ def patched_read_multi_mget(*names)
def pipeline_mget(conn, keys)
keys.each_slice(pipeline_batch_size).flat_map do |subset|
- conn.pipelined do |p|
+ Gitlab::Redis::CrossSlot::Pipeline.new(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
new file mode 100644
index 0000000000000..145ce35a31736
--- /dev/null
+++ b/lib/gitlab/patch/redis_cluster.rb
@@ -0,0 +1,21 @@
+# 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
deleted file mode 100644
index 81636ed665dc5..0000000000000
--- a/lib/gitlab/patch/redis_store_factory.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# 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
new file mode 100644
index 0000000000000..e302d844078d7
--- /dev/null
+++ b/lib/gitlab/patch/slot_loader.rb
@@ -0,0 +1,19 @@
+# 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
deleted file mode 100644
index d660c782d4d01..0000000000000
--- a/lib/gitlab/redis/cluster_store.rb
+++ /dev/null
@@ -1,81 +0,0 @@
-# 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 99f337749d0d8..9e307940de3d1 100644
--- a/lib/gitlab/redis/cluster_util.rb
+++ b/lib/gitlab/redis/cluster_util.rb
@@ -13,14 +13,14 @@ def cluster?(obj)
if obj.is_a?(MultiStore)
cluster?(obj.primary_store) || cluster?(obj.secondary_store)
else
- obj.is_a?(::Redis::Cluster)
+ obj.respond_to?(:_client) && obj._client.is_a?(::Redis::Cluster)
end
end
def batch_unlink(keys, redis)
expired_count = 0
keys.each_slice(1000) do |subset|
- expired_count += redis.pipelined do |pipeline|
+ expired_count += Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
subset.each { |key| pipeline.unlink(key) }
end.sum
end
@@ -30,7 +30,7 @@ def batch_unlink(keys, redis)
# Redis cluster alternative to mget
def batch_get(keys, redis)
keys.each_slice(1000).flat_map do |subset|
- redis.pipelined do |pipeline|
+ Gitlab::Redis::CrossSlot::Pipeline.new(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
deleted file mode 100644
index 99d26760b3d37..0000000000000
--- a/lib/gitlab/redis/command_builder.rb
+++ /dev/null
@@ -1,53 +0,0 @@
-# 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
new file mode 100644
index 0000000000000..e5aa6d9ce7246
--- /dev/null
+++ b/lib/gitlab/redis/cross_slot.rb
@@ -0,0 +1,141 @@
+# 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 476077cb96f0d..e6262f7f61b51 100644
--- a/lib/gitlab/redis/multi_store.rb
+++ b/lib/gitlab/redis/multi_store.rb
@@ -432,6 +432,16 @@ def same_redis_store?
# 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|
@@ -452,7 +462,7 @@ def with_instance(instance, *params)
end
def redis_store?(pool)
- pool.with { |c| c.instance_of?(Gitlab::Redis::MultiStore) || c.is_a?(::Redis) || c.is_a?(::Redis::Cluster) }
+ pool.with { |c| c.instance_of?(Gitlab::Redis::MultiStore) || c.is_a?(::Redis) }
end
def validate_stores!
diff --git a/lib/gitlab/redis/wrapper.rb b/lib/gitlab/redis/wrapper.rb
index f7a7e703d90e7..a106ed3884ef3 100644
--- a/lib/gitlab/redis/wrapper.rb
+++ b/lib/gitlab/redis/wrapper.rb
@@ -20,7 +20,7 @@ class Wrapper
CommandExecutionError = Class.new(StandardError)
class << self
- delegate :params, :url, :store, :encrypted_secrets, to: :new
+ delegate :params, :url, :store, :encrypted_secrets, :redis_client_params, to: :new
def with
pool.with { |redis| yield redis }
@@ -90,17 +90,7 @@ def instrumentation_class
end
def redis
- 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
+ ::Redis.new(params)
end
end
@@ -109,8 +99,13 @@ def initialize(rails_env = nil)
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
@@ -119,14 +114,14 @@ def params
# 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
@@ -139,7 +134,7 @@ def url
end
def db
- redis_store_options[:db] || 0
+ redis_store_options[:db]
end
def sentinels
@@ -161,7 +156,7 @@ def sentinels?
end
def store(extras = {})
- ::Redis::Store::Factory.create(params.merge(extras))
+ ::Redis::Store::Factory.create(redis_store_options.merge(extras))
end
def encrypted_secrets
@@ -187,11 +182,7 @@ def redis_store_options
final_config = parse_extra_config(decrypted_config)
result = if final_config[:cluster].present?
- 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[:db] = 0 # Redis Cluster only supports db 0
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 6da5556833f2e..fab0e9e09e8da 100644
--- a/lib/gitlab/repository_hash_cache.rb
+++ b/lib/gitlab/repository_hash_cache.rb
@@ -86,8 +86,6 @@ def write(key, hash)
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 967a139e7fb3c..eb73a0a3d31eb 100644
--- a/lib/gitlab/set_cache.rb
+++ b/lib/gitlab/set_cache.rb
@@ -62,7 +62,7 @@ def try_include?(key, value)
with do |redis|
redis.multi do |multi|
- multi.sismember(full_key, value.to_s)
+ multi.sismember(full_key, value)
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 c40f125db40d4..883e1ba0558dd 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 @@ def update_latest_wal_location!
argv = []
job_wal_locations.each do |connection_name, location|
diff = pg_wal_lsn_diff(connection_name)
- argv += [connection_name, diff ? diff.to_f : '', location]
+ argv += [connection_name, diff || '', location]
end
with_redis { |r| r.eval(UPDATE_WAL_COOKIE_SCRIPT, keys: [cookie_key], argv: argv) }
diff --git a/spec/controllers/concerns/product_analytics_tracking_spec.rb b/spec/controllers/concerns/product_analytics_tracking_spec.rb
index 247be5429a1df..7b48782be989e 100644
--- a/spec/controllers/concerns/product_analytics_tracking_spec.rb
+++ b/spec/controllers/concerns/product_analytics_tracking_spec.rb
@@ -109,8 +109,7 @@ def expect_no_internal_tracking
end
it 'tracks total Redis counters' do
- expect(Gitlab::Usage::Metrics::Instrumentations::TotalCountMetric).to receive(:redis_key)
- .twice.and_call_original # total and 7d
+ expect(Gitlab::Usage::Metrics::Instrumentations::TotalCountMetric).to receive(:redis_key).twice # 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
deleted file mode 100644
index f583658624738..0000000000000
--- a/spec/fixtures/config/redis_new_format_host_standalone.yml
+++ /dev/null
@@ -1,8 +0,0 @@
-# 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 d89ad21ca82bd..cf82fd751ddea 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 @@
sub = ActionCable.server.pubsub.send(:redis_connection)
- expect(sub.connection[:id]).to eq('/home/localuser/redis/redis.socket/0')
+ expect(sub.connection[:id]).to eq('unix:///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 bd943830f6e87..c9333d022dd31 100644
--- a/spec/initializers/session_store_spec.rb
+++ b/spec/initializers/session_store_spec.rb
@@ -15,7 +15,7 @@
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_server: Gitlab::Redis::Sessions.params.merge(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE)))
+ expect(subject).to receive(:session_store).with(:redis_store, a_hash_including(redis_store: kind_of(::Redis::Store)))
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 17e58a4ddef60..5176cc752666a 100644
--- a/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
+++ b/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
@@ -5,8 +5,6 @@
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
@@ -83,7 +81,7 @@ def migration
end
around do |example|
- described_class.register_running_worker(worker_class, worker_id) do
+ described_class.register_running_worker(worker_class, anything) 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 a55ae5711a8d0..c52d1b4c9f2a5 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 @@
RSpec.describe Gitlab::BackgroundMigration::Redis::BackfillProjectPipelineStatusTtl,
:clean_gitlab_redis_cache, feature_category: :redis do
- let(:redis) { ::Gitlab::Redis::Cache.redis }
+ let(:redis) { ::Redis.new(::Gitlab::Redis::Cache.params) }
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 9f1ade529b182..cfc3fb398e24e 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 @@
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 8068be798d392..30981e4bd7d11 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 @@ def fake_file(offset)
mapping.each do |key, value|
full_key = described_class.cache_key_for(key)
- found_key = Gitlab::Redis::Cache.with { |r| r.get(full_key).force_encoding("UTF-8") }
+ found_key = Gitlab::Redis::Cache.with { |r| r.get(full_key) }
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 3dc23422e4edc..186bf7d7ec15f 100644
--- a/spec/lib/gitlab/external_authorization/cache_spec.rb
+++ b/spec/lib/gitlab/external_authorization/cache_spec.rb
@@ -23,9 +23,9 @@ def set_in_redis(key, value)
describe '#load' do
it 'reads stored info from redis' do
freeze_time do
- set_in_redis(:access, false.to_s)
+ set_in_redis(:access, false)
set_in_redis(:reason, 'Access denied for now')
- set_in_redis(:refreshed_at, Time.now.to_s)
+ set_in_redis(:refreshed_at, Time.now)
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 2eb77add2ed69..a8bded69696e6 100644
--- a/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
+++ b/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
@@ -9,9 +9,10 @@
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_store_class.with(&:flushdb)
+ redis_client.call("flushdb")
end
describe 'read and write' do
@@ -23,30 +24,27 @@
# 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
+ # Exercise counting empty multi bulk reply. Returns an empty hash `{}`
+ [] | [:hgetall, 'foobar'] | (7 + 6) | 2
# 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)
+ # 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)
# Exercise counting of a bulk reply
[[:set, 'foo', 'bar' * 100]] | [:get, 'foo'] | (3 + 3) | (3 * 100)
- # Nested array response: [['foo', 0], ['bar', 1.1000000000000001]] due to Redis precision
- # See https://github.com/redis/redis/issues/1499
+ # Nested array response: [['foo', 0.0], ['bar', 1.0]]. Returns scores as float.
[[:zadd, 'myset', 0, 'foo'],
- [:zadd, 'myset', 1.1,
- 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | (6 + 5 + 1 + 2 + 10) | (3 + 1 + 3 + 18)
+ [:zadd, 'myset', 1, 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | (6 + 5 + 1 + 2 + 10) | (3 + 3 + 3 + 3)
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
+ setup.each { |cmd| redis_client.call(*cmd) }
+ RequestStore.clear!
+ redis_client.call(*command)
expect(Gitlab::Instrumentation::Redis.read_bytes).to eq(expect_read)
expect(Gitlab::Instrumentation::Redis.write_bytes).to eq(expect_write)
@@ -60,48 +58,35 @@
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') }
+ redis_client.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
+ redis_client.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:get, '{foobar}baz')
end
end
context 'when encountering exceptions' do
- 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
+ before do
+ allow(redis_client.instance_variable_get(:@raw_connection)).to receive(:call).and_raise(
+ RedisClient::Error)
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.instance_variable_get(:@raw_connection)).to receive(:call).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
+ 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
- expect do
- redis_store_class.with { |redis| redis.call(:auth, 'foo', 'bar') }
- end.to raise_exception(Redis::CommandError)
- end
+ expect do
+ redis_client.call(:auth, 'foo', 'bar')
+ end.to raise_error(RedisClient::Error)
end
end
@@ -114,7 +99,7 @@
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') }
+ redis_client.call(:mget, 'foo', 'bar')
end
it 'does not count allowed cross-slot requests' do
@@ -122,7 +107,7 @@
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') }
+ redis_client.call(:mget, 'foo', 'bar')
end
end
@@ -131,7 +116,7 @@
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') }
+ redis_client.call(:mget, 'bar')
end
end
@@ -139,7 +124,7 @@
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') }
+ redis_client.call(:mget, '{foo}bar', '{foo}baz')
end
end
@@ -150,7 +135,7 @@
it 'still runs cross-slot validation' do
expect do
- redis_store_class.with { |redis| redis.mget('foo', 'bar') }
+ redis_client.call('mget', 'foo', 'bar')
end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
end
end
@@ -172,7 +157,7 @@
expect(instrumentation_class).to receive(:instance_observe_duration).with(a_value > 0)
.and_call_original
- redis_store_class.with { |redis| redis.call(*command) }
+ redis_client.call(*command)
end
end
@@ -181,21 +166,17 @@
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
+ redis_client.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:get, '{foobar}baz')
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
+ redis_client.pipelined do |pipeline|
+ pipeline.call(:get, 'foo')
+ pipeline.call(:get, 'bar')
end
end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
end
@@ -219,11 +200,11 @@
with_them do
it 'skips requests we do not want in the apdex' do
- setup.each { |cmd| redis_store_class.with { |redis| redis.call(*cmd) } }
+ setup.each { |cmd| redis_client.call(*cmd) }
expect(instrumentation_class).not_to receive(:instance_observe_duration)
- redis_store_class.with { |redis| redis.call(*command) }
+ redis_client.call(*command)
end
end
@@ -231,12 +212,10 @@
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
+ redis_client.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:rpush, '{foobar}baz', 1)
+ pipeline.call(:brpop, '{foobar}baz', 0)
end
end
end
diff --git a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
new file mode 100644
index 0000000000000..2a160a9d31662
--- /dev/null
+++ b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
@@ -0,0 +1,261 @@
+# 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
new file mode 100644
index 0000000000000..000083fc6d06b
--- /dev/null
+++ b/spec/lib/gitlab/patch/node_loader_spec.rb
@@ -0,0 +1,80 @@
+# 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 ae9c31e0c513a..21c256fdbbee3 100644
--- a/spec/lib/gitlab/patch/redis_cache_store_spec.rb
+++ b/spec/lib/gitlab/patch/redis_cache_store_spec.rb
@@ -13,8 +13,6 @@
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
@@ -36,7 +34,7 @@
end
context 'when reading large amount of keys' do
- let(:input_size) { 2100 }
+ let(:input_size) { 2000 }
let(:chunk_size) { 1000 }
shared_examples 'read large amount of keys' do
@@ -47,11 +45,10 @@
::Gitlab::Redis::ClusterUtil.cluster?(redis.default_store)
if normal_cluster || multistore_cluster
- 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
+ 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
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 3e2d46dd19c8c..af094e9e0d251 100644
--- a/spec/lib/gitlab/patch/redis_client_spec.rb
+++ b/spec/lib/gitlab/patch/redis_client_spec.rb
@@ -6,7 +6,7 @@
include RedisHelpers
let_it_be(:redis_store_class) { define_helper_redis_store_class }
- let_it_be(:redis_client) { RedisClient.new(redis_store_class.params) }
+ let_it_be(:redis_client) { RedisClient.new(redis_store_class.redis_client_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
deleted file mode 100644
index be37a044ac154..0000000000000
--- a/spec/lib/gitlab/patch/redis_store_factory_spec.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-# 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 efe6f9382f954..19b3f239d915f 100644
--- a/spec/lib/gitlab/rack_attack/store_spec.rb
+++ b/spec/lib/gitlab/rack_attack/store_spec.rb
@@ -102,7 +102,7 @@ def with_redis(&block)
before do
broken_redis = Redis.new(
url: 'redis://127.0.0.0:0',
- custom: { instrumentation_class: Gitlab::Redis::RateLimiting.instrumentation_class }
+ 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
deleted file mode 100644
index aaeee6156a0d8..0000000000000
--- a/spec/lib/gitlab/redis/cluster_store_spec.rb
+++ /dev/null
@@ -1,116 +0,0 @@
-# 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 1cd6a450584bb..a517545014508 100644
--- a/spec/lib/gitlab/redis/cluster_util_spec.rb
+++ b/spec/lib/gitlab/redis/cluster_util_spec.rb
@@ -5,14 +5,10 @@
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
@@ -21,7 +17,10 @@
end
before do
- allow(router_stub).to receive(:node_keys).and_return([])
+ # 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(Gitlab::Redis::MultiStore).to receive(:same_redis_store?).and_return(false)
skip_default_enabled_yaml_check
@@ -29,8 +28,8 @@
with_them do
it 'returns expected value' do
- 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_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_pool = ConnectionPool.new { primary_redis }
secondary_pool = ConnectionPool.new { secondary_redis }
multistore = Gitlab::Redis::MultiStore.new(primary_pool, secondary_pool, 'teststore')
@@ -49,8 +48,16 @@
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::Cluster.new(nodes: ['redis://localhost:6000']))).to be_truthy
+ expect(described_class.cluster?(::Redis.new(cluster: ['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
deleted file mode 100644
index 7ebb0228d2d7a..0000000000000
--- a/spec/lib/gitlab/redis/command_builder_spec.rb
+++ /dev/null
@@ -1,57 +0,0 @@
-# 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
new file mode 100644
index 0000000000000..4e9830f4110a2
--- /dev/null
+++ b/spec/lib/gitlab/redis/cross_slot_spec.rb
@@ -0,0 +1,134 @@
+# 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 0d841be4e3c64..348215d553c03 100644
--- a/spec/lib/gitlab/redis/multi_store_spec.rb
+++ b/spec/lib/gitlab/redis/multi_store_spec.rb
@@ -58,7 +58,6 @@
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
@@ -70,7 +69,6 @@
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
@@ -620,6 +618,35 @@
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 e822b7399b741..874822e3e6acc 100644
--- a/spec/lib/gitlab/redis/sessions_spec.rb
+++ b/spec/lib/gitlab/redis/sessions_spec.rb
@@ -8,9 +8,9 @@
describe '#store' do
subject(:store) { described_class.store(namespace: described_class::SESSION_NAMESPACE) }
- # Check that Gitlab::Redis::Sessions is configured as RedisStore or ClusterStore
+ # Check that Gitlab::Redis::Sessions is configured as RedisStore.
it 'instantiates an instance of Redis::Store' do
- expect([::Redis::Store, ::Gitlab::Redis::ClusterStore].include?(store.class)).to eq(true)
+ expect(store).to be_instance_of(::Redis::Store)
end
end
end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index a4303a6e0fced..c3e9f5a178e91 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -544,18 +544,18 @@ def initialize_from_file_path(path)
Rack::Test::UploadedFile.prepend(TouchRackUploadedFile)
-# Inject middleware to enable ActiveSupport::Notifications for Redis commands
+# Monkey-patch to enable ActiveSupport::Notifications for Redis commands
module RedisCommands
module Instrumentation
- def call(command, redis_config)
- ActiveSupport::Notifications.instrument('redis.process_commands', commands: command) do
- super(command, redis_config)
+ def process(commands, &block)
+ ActiveSupport::Notifications.instrument('redis.process_commands', commands: commands) do
+ super(commands, &block)
end
end
end
end
-RedisClient.register(RedisCommands::Instrumentation)
+Redis::Client.prepend(RedisCommands::Instrumentation)
module UsersInternalAllowExclusiveLease
extend ActiveSupport::Concern
diff --git a/spec/support/helpers/dns_helpers.rb b/spec/support/helpers/dns_helpers.rb
index 27310c7e04c96..0250e4326099e 100644
--- a/spec/support/helpers/dns_helpers.rb
+++ b/spec/support/helpers/dns_helpers.rb
@@ -60,8 +60,8 @@ def db_hosts
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.flat_map do |redis_instance|
- redis_instance.params[:host] || redis_instance.params[:nodes]&.map { |n| n[:host] }
+ hosts = Gitlab::Redis::ALL_CLASSES.map do |redis_instance|
+ redis_instance.redis_client_params[: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 2cb92089723a9..2b1e1ebad23a8 100644
--- a/spec/support/matchers/exceed_redis_call_limit.rb
+++ b/spec/support/matchers/exceed_redis_call_limit.rb
@@ -14,7 +14,7 @@ def verify_count(expected, block)
end
def verify_commands_count(command, expected, block)
- @actual = build_recorder(block).by_command(command.to_s).count
+ @actual = build_recorder(block).by_command(command).count
@actual > expected
end
diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml
index 7e113122eacc6..8791220f11e81 100644
--- a/spec/support/rspec_order_todo.yml
+++ b/spec/support/rspec_order_todo.yml
@@ -6292,6 +6292,7 @@
- './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 55ce520caab5b..3dbe43d822f47 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 @@
expect(Gitlab::JobWaiter).to receive(:notify).with(waiter_key, anything)
- worker.class.perform_inline(project_id, {}, waiter_key)
+ worker.perform(project_id, {}, waiter_key)
end
end
@@ -49,7 +49,7 @@
expect(Gitlab::BitbucketImport::Logger).to receive(:info).twice
expect_next(worker.importer_class, project, kind_of(Hash)).to receive(:execute)
- worker.class.perform_inline(project_id, {}, waiter_key)
+ worker.perform(project_id, {}, waiter_key)
end
it_behaves_like 'notifies the waiter'
@@ -62,7 +62,7 @@
it 'tracks the error' do
expect(Gitlab::Import::ImportFailureService).to receive(:track).once
- worker.class.perform_inline(project_id, {}, waiter_key)
+ worker.perform(project_id, {}, waiter_key)
end
end
@@ -74,7 +74,7 @@
it 'tracks the error and raises the error' do
expect(Gitlab::Import::ImportFailureService).to receive(:track).once
- expect { worker.class.perform_inline(project_id, {}, waiter_key) }.to raise_error(StandardError)
+ expect { worker.perform(project_id, {}, waiter_key) }.to raise_error(StandardError)
end
end
end
@@ -85,7 +85,7 @@
it 'does not call the importer' do
expect_next(worker.importer_class).not_to receive(:execute)
- worker.class.perform_inline(project_id, {}, waiter_key)
+ worker.perform(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 89eec531b87bc..45248f57683b7 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,10 +19,6 @@
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 2f387528740b6..1c153b7c31b62 100644
--- a/spec/support/shared_examples/redis/redis_shared_examples.rb
+++ b/spec/support/shared_examples/redis/redis_shared_examples.rb
@@ -80,17 +80,18 @@
context 'with new format' do
it_behaves_like 'redis store' do
- # 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(:config_file_name) { config_new_format_host }
let(:host) { "development-host:#{redis_port}" }
end
end
end
- describe '.params' do
- subject { described_class.new(rails_env).params }
+ 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 }
let(:rails_env) { 'development' }
let(:config_file_name) { config_old_format_socket }
@@ -102,6 +103,56 @@
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
@@ -200,16 +251,10 @@
with_them do
it 'returns hash with host, port, db, and password' do
- is_expected.to include(name: host, password: 'mynewpassword', db: redis_database)
+ is_expected.to include(host: host, password: 'mynewpassword', port: redis_port, 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
@@ -227,19 +272,13 @@
it 'returns hash with cluster and password' do
is_expected.to include(
password: 'myclusterpassword',
- nodes: [
+ cluster: [
{ 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 5c200f15bae3b..ef46db5e29ea7 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 4cb0a25f89294..d11b044b093bc 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,24 +108,17 @@
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',
- 'jid' => anything))
+ .with(log_attributes.merge('message' => 'importer failed', 'exception.message' => 'error_message'))
expect(Gitlab::JobWaiter)
.to receive(:notify)
- .with('some_key', anything, ttl: Gitlab::Import::JOB_WAITER_TTL)
+ .with('some_key', subject.jid, ttl: Gitlab::Import::JOB_WAITER_TTL)
- subject.class.perform_inline(user.id, gist_hash, 'some_key') # perform_inline calls .perform
+ subject.perform(user.id, gist_hash, 'some_key')
expect_snowplow_event(
category: 'Gitlab::GithubGistsImport::ImportGistWorker',
@@ -137,7 +130,7 @@
end
it 'persists failure' do
- expect { subject.class.perform_inline(user.id, gist_hash, 'some_key') }
+ expect { subject.perform(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 c9c4c6e2d2b52..3b2114ee48813 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 @@
context 'when import label does not exist' do
it 'does not record import failure' do
- subject.class.perform_inline(project.id, 123, issue_attrs, some_key)
+ subject.perform(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 @@
before do
Gitlab::JiraImport.cache_import_label_id(project.id, label.id)
- subject.class.perform_inline(project.id, 123, issue_attrs, some_key)
+ subject.perform(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 d8f2cacdb13e5..9f29c84a94895 100644
--- a/spec/workers/redis_migration_worker_spec.rb
+++ b/spec/workers/redis_migration_worker_spec.rb
@@ -30,7 +30,7 @@ def scan_match_pattern
end
def redis
- ::Gitlab::Redis::Cache.redis
+ ::Redis.new(::Gitlab::Redis::Cache.params)
end
end
end
--
GitLab