diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml
index 2095d2c1ca3e154681778627fe1d09dc4f1839d3..754d31a99b2dcaa75b89d2322ac19955f3a13b4b 100644
--- a/.rubocop_todo/layout/line_length.yml
+++ b/.rubocop_todo/layout/line_length.yml
@@ -3703,7 +3703,6 @@ 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 5b352842a870d6ed54a4e78abfafc4ccc180a6a7..a7cac8214c08c6721a2bf1fcfd312e294de10cd0 100644
--- a/.rubocop_todo/lint/ambiguous_operator_precedence.yml
+++ b/.rubocop_todo/lint/ambiguous_operator_precedence.yml
@@ -102,7 +102,6 @@ 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 e497e2199abd29ae4b3122a57dd1513165ee217d..7ff9f8b091f7bd0e2d3f7e59982bc1969f86f1c3 100644
--- a/.rubocop_todo/rspec/feature_category.yml
+++ b/.rubocop_todo/rspec/feature_category.yml
@@ -3588,7 +3588,6 @@ 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 4e4aa25c4176fc9c79697caeb7b284c9033271e4..7d7755db97417379e03140837cd321bd39b21874 100644
--- a/.rubocop_todo/rspec/named_subject.yml
+++ b/.rubocop_todo/rspec/named_subject.yml
@@ -2334,7 +2334,6 @@ 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 4bf891cd5c33cde41cd390d958bca3ad847b7395..31f59f51bf7c6d7ead47159da0da68dddcc0c798 100644
--- a/.rubocop_todo/style/inline_disable_annotation.yml
+++ b/.rubocop_todo/style/inline_disable_annotation.yml
@@ -2517,7 +2517,6 @@ 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'
@@ -2556,7 +2555,6 @@ 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'
@@ -2573,7 +2571,6 @@ 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'
@@ -2942,9 +2939,7 @@ 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 6c452d26358a99ff1c0893ab2e9a6ac0cebd9d13..506904a1d7bf787a49af591f726edf35924a1911 100644
--- a/Gemfile
+++ b/Gemfile
@@ -288,8 +288,9 @@ gem 'js_regex', '~> 3.8' # rubocop:todo Gemfile/MissingFeatureCategory
gem 'device_detector' # rubocop:todo Gemfile/MissingFeatureCategory
# Redis
-gem 'redis', '~> 4.8.0' # rubocop:todo Gemfile/MissingFeatureCategory
-gem 'redis-namespace', '~> 1.10.0' # rubocop:todo Gemfile/MissingFeatureCategory
+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 'connection_pool', '~> 2.4' # rubocop:todo Gemfile/MissingFeatureCategory
# Redis session store
diff --git a/Gemfile.checksum b/Gemfile.checksum
index 6edc3cfb8f1473216a421c30784708acddf63e8c..ae18a1d7c833b0b6916387cb9067392e6e77e8be 100644
--- a/Gemfile.checksum
+++ b/Gemfile.checksum
@@ -523,9 +523,11 @@
{"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":"4.8.0","platform":"ruby","checksum":"2000cf5014669c9dc821704b6d322a35a9a33852a95208911d9175d63b448a44"},
+{"name":"redis","version":"5.0.8","platform":"ruby","checksum":"3b770ea597850b26d6a9718fa184241e53e6c8a7ae0486ee8bfaefd29f26f3d8"},
{"name":"redis-actionpack","version":"5.4.0","platform":"ruby","checksum":"f10cf649ab05914716d63334d7f709221ecc883b87cf348f90ecfe0c35ea3540"},
{"name":"redis-client","version":"0.19.0","platform":"ruby","checksum":"6ed9af23ff5aa87cf4d59439db77082b4cae5a0abbdd114ec5420bd63456324d"},
+{"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 087da2cb58d566b531bddfe639a4962a851fbd69..1429f0c39dabbe562a91218ade8f3ebdb9627568 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1385,13 +1385,19 @@ GEM
json
recursive-open-struct (1.1.3)
redcarpet (3.6.0)
- redis (4.8.0)
+ redis (5.0.8)
+ redis-client (>= 0.17.0)
redis-actionpack (5.4.0)
actionpack (>= 5, < 8)
redis-rack (>= 2.1.0, < 4)
redis-store (>= 1.1.0, < 2)
redis-client (0.19.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)
@@ -2069,8 +2075,9 @@ DEPENDENCIES
rbtrace (~> 0.4)
re2 (= 2.7.0)
recaptcha (~> 5.12)
- redis (~> 4.8.0)
+ redis (~> 5.0.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 2eb9c9bca7f24730c3de04caf629006261b51368..756e0b7fbf9cddf3ca8f58d75503f479e54496bf 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
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ 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 b4d884f914df027af9aa0c87a29205b1246b9959..2ac3c5e991bfb79b3faab1acb160839de8f4f040 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, max_jids])
+ redis.eval(LUA_REGISTER_SCRIPT, keys: [counter_key], argv: [jid.to_s, max_jids.to_i])
end.present?
end
@@ -59,7 +59,7 @@ def counter_key
end
def remove_job_keys(redis, keys)
- redis.srem?(counter_key, keys)
+ redis.srem?(counter_key, keys) if keys.present?
end
def with_redis(&block)
diff --git a/config/initializers/7_redis.rb b/config/initializers/7_redis.rb
index fbd2cbaabad64847720c12be3f8ea4cdea434876..c7385612602a02f16defc4a878dcb06bdfd5a71f 100644
--- a/config/initializers/7_redis.rb
+++ b/config/initializers/7_redis.rb
@@ -21,12 +21,6 @@
# :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 dc69a108f56e5f3386b3b67f7c50a5b560ca47ce..1d22c774443068e6981af83fe6cf7b5f5fa00cf0 100644
--- a/config/initializers/action_cable.rb
+++ b/config/initializers/action_cable.rb
@@ -25,7 +25,17 @@
# 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(instrumentation_class: ::Gitlab::Instrumentation::Redis::ActionCable)
+ .merge(custom: { instrumentation_class: "ActionCable" })
+
+ if args[:url]
+ # cable.yml uses the url key but unix sockets needs to be passed into Redis
+ # under the `path` key. We do a simple reassignment to resolve that.
+ uri = URI.parse(args[:url])
+ if uri.scheme == 'unix'
+ args[:path] = uri.path
+ args.delete(:url)
+ end
+ end
::Redis.new(args)
end
diff --git a/config/initializers/peek.rb b/config/initializers/peek.rb
index e1c59851fb1a95f26d6134639e15cea3066668ad..72e7d3f10f98b69bece9a7809006d8bd95001ed8 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: ::Redis.new(Gitlab::Redis::Cache.params),
+ client: Gitlab::Redis::Cache.redis,
expires_in: 5.minutes
}
diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb
index 7f410d7bf7bbd97b2be9e74130d799746657cbc7..70bd3d012d096f5e72e0c272adc1d03813161af1 100644
--- a/config/initializers/session_store.rb
+++ b/config/initializers/session_store.rb
@@ -29,12 +29,12 @@
"_gitlab_session"
end
-store = Gitlab::Redis::Sessions.store(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE)
+::Redis::Store::Factory.prepend(Gitlab::Patch::RedisStoreFactory)
Rails.application.configure do
config.session_store(
:redis_store, # Using the cookie_store would enable session replay attacks.
- redis_store: store,
+ redis_server: Gitlab::Redis::Sessions.params.merge(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE),
key: cookie_key,
secure: Gitlab.config.gitlab.https,
httponly: true,
diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb
index 9b7233dbd14e2cf051b5fac14594cc5bc9fcfcc4..401d18df790d8d8ee66f3ed804e79766c5d5f6c7 100644
--- a/config/initializers/sidekiq.rb
+++ b/config/initializers/sidekiq.rb
@@ -28,7 +28,7 @@ def enable_semi_reliable_fetch_mode?
end
# Custom Queues configuration
-queues_config_hash = Gitlab::Redis::Queues.redis_client_params
+queues_config_hash = Gitlab::Redis::Queues.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 aebb3004223566bea7d81e27c031b3964b29463d..7ec6414bf45ed7e346c774f04053f29e76a48295 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 = Redis.new(Gitlab::Redis::SharedState.params)
+ redis = Gitlab::Redis::SharedState.redis
redis.info
```
diff --git a/doc/development/redis.md b/doc/development/redis.md
index f9afe5e071890ea7bb5370229334cc22fbb899ca..368f6709367929ea9be10bfc1cb645e3c126aa32 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 [`Gitlab::Redis::CrossSlot::Pipeline`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/redis/cross_slot.rb) wrapper.
+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.
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 `Gitlab::Redis::CrossSlot::Pipeline` wrapper.
+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.
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 ff5394cef8f22e1fadf8f6003e6b7848234c320d..054364fce5e44c07dd8f8dc65accad20f77c91ba 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 = ::Redis.new(params)
- secondary_store = ::Redis.new(config_fallback.params)
+ primary_store = init_redis(params)
+ secondary_store = init_redis(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 50ececd4710628ba472a4a35ad08e87b0dbdbcef..1b82c2f3a76a82385393ccb5af8a5bed2576e3de 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
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |p|
+ redis.pipelined do |p|
p.del(redis_score_key)
p.del(redis_set_key)
end
diff --git a/lib/gitlab/auth/unique_ips_limiter.rb b/lib/gitlab/auth/unique_ips_limiter.rb
index 341edbed9c2d683d68d281a7c9a9dee3689301b3..0c4a5147a384835d836c4989942d7128bf4d644b 100644
--- a/lib/gitlab/auth/unique_ips_limiter.rb
+++ b/lib/gitlab/auth/unique_ips_limiter.rb
@@ -31,7 +31,7 @@ def update_and_return_ips_count(user_id, ip)
Gitlab::Redis::SharedState.with do |redis|
redis.multi do |r|
- r.zadd(key, time, ip)
+ r.zadd(key, time, ip) if ip
r.zremrangebyscore(key, 0, time - config.unique_ips_limit_time_window)
r.zcard(key)
end.last
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 2672498b6271f6b14f56ace976d02699bf0d787f..7886abdb6d40800c6456b165413d7e3d20e003f9 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
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
keys.each { |key| pipeline.expire(key, ttl_duration + rand(-ttl_jitter..ttl_jitter)) }
end
end
@@ -25,7 +25,7 @@ def scan_match_pattern
end
def redis
- @redis ||= ::Redis.new(Gitlab::Redis::Cache.params)
+ @redis ||= Gitlab::Redis::Cache.redis
end
end
end
diff --git a/lib/gitlab/cache/ci/project_pipeline_status.rb b/lib/gitlab/cache/ci/project_pipeline_status.rb
index 4beb8f54abf034bc2912e55cd1a39db1968bd82b..9320233501f168f61af8538920f6c9445e38ab94 100644
--- a/lib/gitlab/cache/ci/project_pipeline_status.rb
+++ b/lib/gitlab/cache/ci/project_pipeline_status.rb
@@ -99,7 +99,7 @@ def load_from_cache
def store_in_cache
with_redis do |redis|
redis.pipelined do |p|
- p.mapped_hmset(cache_key, { sha: sha, status: status, ref: ref })
+ p.mapped_hmset(cache_key, { sha: sha.to_s, status: status.to_s, ref: ref.to_s })
p.expire(cache_key, STATUS_KEY_TTL)
end
end
diff --git a/lib/gitlab/cache/import/caching.rb b/lib/gitlab/cache/import/caching.rb
index c538e5162c7b3036c62495ea91ef30552f568141..0b24137076dd662d6962516e43c376e8823f90a5 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)
+ redis.sismember(key, value || value.to_s)
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
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
mapping.each do |raw_key, value|
key = cache_key_for("#{key_prefix}#{raw_key}")
diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index dc5f4e1b3243247bb25b8b68f8ec8ff87803e364..0d5a66175f8e520f9de489336a48e6af5ec62018 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -197,7 +197,9 @@ def read_cache
record_hit_ratio(results)
results.map! do |result|
- Gitlab::Json.parse(gzip_decompress(result), symbolize_names: true) unless result.nil?
+ unless result.nil?
+ Gitlab::Json.parse(gzip_decompress(result.force_encoding(Encoding::UTF_8)), symbolize_names: true)
+ end
end
file_paths.zip(results).to_h
diff --git a/lib/gitlab/discussions_diff/highlight_cache.rb b/lib/gitlab/discussions_diff/highlight_cache.rb
index 18ff7c28e174b6f29a4db719877a42bdf2241f70..dbdc8c3c95c7ca6f618cdf0749ea8dcfecf1238e 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
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipelined|
+ redis.pipelined do |pipelined|
mapping.each do |raw_key, value|
key = cache_key_for(raw_key)
@@ -42,7 +42,7 @@ def read_multiple(raw_keys)
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if Gitlab::Redis::ClusterUtil.cluster?(redis)
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
keys.each { |key| pipeline.get(key) }
end
else
@@ -54,7 +54,7 @@ def read_multiple(raw_keys)
content.map! do |lines|
next unless lines
- Gitlab::Json.parse(gzip_decompress(lines)).map! do |line|
+ Gitlab::Json.parse(gzip_decompress(lines.force_encoding(Encoding::UTF_8))).map! do |line|
Gitlab::Diff::Line.safe_init_from_hash(line)
end
end
diff --git a/lib/gitlab/etag_caching/store.rb b/lib/gitlab/etag_caching/store.rb
index 5fdf5ac9436275ffa5721545658598e949bb2dd7..57c160c9607e74b9ea58c437c4e35097af2c2bcd 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|
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
keys.each_with_index do |key, i|
pipeline.set(redis_shared_state_key(key), etags[i], ex: EXPIRY_TIME, nx: only_if_missing)
end
diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb
index 0b18a33770701353fb385d29408e9eb22f7560c3..bae1bf061433e24093174fbdedd8dbfbd4dfd0fd 100644
--- a/lib/gitlab/exclusive_lease.rb
+++ b/lib/gitlab/exclusive_lease.rb
@@ -60,6 +60,7 @@ 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])
@@ -110,7 +111,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])
+ result = redis.eval(LUA_RENEW_SCRIPT, keys: [@redis_shared_state_key], argv: [@uuid, @timeout.to_i])
result == @uuid
end
end
diff --git a/lib/gitlab/instrumentation/redis_cluster_validator.rb b/lib/gitlab/instrumentation/redis_cluster_validator.rb
index 948132e6edddb973e1ad926884f73486efe64014..5c3655f8f803f0d203585e2e292aa730800d1c12 100644
--- a/lib/gitlab/instrumentation/redis_cluster_validator.rb
+++ b/lib/gitlab/instrumentation/redis_cluster_validator.rb
@@ -1,7 +1,7 @@
# frozen_string_literal: true
require 'rails'
-require 'redis'
+require 'redis-clustering'
module Gitlab
module Instrumentation
@@ -230,7 +230,7 @@ def has_cross_slot_keys?(keys)
end
def key_slot(key)
- ::Redis::Cluster::KeySlotConverter.convert(extract_hash_tag(key))
+ ::RedisClient::Cluster::KeySlotConverter.convert(extract_hash_tag(key))
end
# This is almost identical to Redis::Cluster::Command#extract_hash_tag,
diff --git a/lib/gitlab/instrumentation/redis_interceptor.rb b/lib/gitlab/instrumentation/redis_interceptor.rb
deleted file mode 100644
index 9c89af6a0dc5814b1a156faab19171be0b3dde5f..0000000000000000000000000000000000000000
--- a/lib/gitlab/instrumentation/redis_interceptor.rb
+++ /dev/null
@@ -1,55 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Instrumentation
- module RedisInterceptor
- include RedisHelper
-
- def call(command)
- instrument_call([command], instrumentation_class) do
- super
- end
- end
-
- def call_pipeline(pipeline)
- instrument_call(pipeline.commands, instrumentation_class, true) do
- super
- end
- end
-
- def write(command)
- measure_write_size(command, instrumentation_class) if ::RequestStore.active?
- super
- end
-
- def read
- result = super
- measure_read_size(result, instrumentation_class) if ::RequestStore.active?
- result
- end
-
- def ensure_connected
- super do
- instrument_reconnection_errors do
- yield
- end
- end
- end
-
- def instrument_reconnection_errors
- yield
- rescue ::Redis::BaseConnectionError => ex
- instrumentation_class.instance_count_connection_exception(ex)
-
- raise ex
- end
-
- # That's required so it knows which GitLab Redis instance
- # it's interacting with in order to categorize accordingly.
- #
- def instrumentation_class
- @options[:instrumentation_class] # rubocop:disable Gitlab/ModuleWithInstanceVariables
- end
- end
- end
-end
diff --git a/lib/gitlab/issues/rebalancing/state.rb b/lib/gitlab/issues/rebalancing/state.rb
index c60dac6f571e5e393bfa6438af86c309bb84bf59..12cc5f6e5ddf4d7dde5c242914e2831a0e76cd24 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
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
pipeline.expire(issue_ids_key, REDIS_EXPIRY_TIME)
pipeline.expire(current_index_key, REDIS_EXPIRY_TIME)
pipeline.expire(current_project_key, REDIS_EXPIRY_TIME)
diff --git a/lib/gitlab/markdown_cache/redis/store.rb b/lib/gitlab/markdown_cache/redis/store.rb
index af9098c3300dea7564c3a4ea9239ead7a35cfe62..85aeaba92c4a5aee21e6b3c415903b4cb78dd2b3 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
- Gitlab::Redis::CrossSlot::Pipeline.new(r).pipelined do |pipeline|
+ r.pipelined do |pipeline|
subjects.each do |subject|
new(subject).read(pipeline)
end
diff --git a/lib/gitlab/patch/command_loader.rb b/lib/gitlab/patch/command_loader.rb
deleted file mode 100644
index 357b6270b0dd597db0be6012580a3204789a88f6..0000000000000000000000000000000000000000
--- a/lib/gitlab/patch/command_loader.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Patch
- module CommandLoader
- extend ActiveSupport::Concern
-
- class_methods do
- # Shuffle the node list to spread out initial connection creation amongst all nodes
- #
- # The input is a Redis::Cluster::Node instance which is an Enumerable.
- # `super` receives an Array of Redis::Client instead of a Redis::Cluster::Node
- def load(nodes)
- super(nodes.to_a.shuffle)
- end
- end
- end
- end
-end
diff --git a/lib/gitlab/patch/node_loader.rb b/lib/gitlab/patch/node_loader.rb
deleted file mode 100644
index 85237abc1376b9340eee3df8ad0087049ea1d543..0000000000000000000000000000000000000000
--- a/lib/gitlab/patch/node_loader.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-# frozen_string_literal: true
-
-# Patch to address https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/2212#note_1287996694
-# It uses hostname instead of IP address if the former is present in `CLUSTER NODES` output.
-if Gem::Version.new(Redis::VERSION) > Gem::Version.new('4.8.1')
- raise 'New version of redis detected, please remove or update this patch'
-end
-
-module Gitlab
- module Patch
- module NodeLoader
- extend ActiveSupport::Concern
-
- class_methods do
- # Shuffle the node list to spread out initial connection creation amongst all nodes
- #
- # The input is a Redis::Cluster::Node instance which is an Enumerable.
- # `super` receives an Array of Redis::Client instead of a Redis::Cluster::Node
- def load_flags(nodes)
- super(nodes.to_a.shuffle)
- end
- end
-
- def self.prepended(base)
- base.class_eval do
- # monkey-patches https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/cluster/node_loader.rb#L23
- def self.fetch_node_info(node)
- node.call(%i[cluster nodes]).split("\n").map(&:split).to_h do |arr|
- [
- extract_host_identifier(arr[1]),
- (arr[2].split(',') & %w[master slave]).first # rubocop:disable Naming/InclusiveLanguage
- ]
- end
- end
-
- # Since `CLUSTER SLOT` uses the preferred endpoint determined by
- # the `cluster-preferred-endpoint-type` config value, we will prefer hostname over IP address.
- # See https://redis.io/commands/cluster-nodes/ for details on the output format.
- #
- # @param [String] Address info matching fhe format: <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 96729056ce5f765ca5c9a5ca1c7c00806e5b3776..35c617ba72b1ce671bdc723b5dbf425b2e351b70 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 += Gitlab::Redis::CrossSlot::Pipeline.new(conn).pipelined do |pipeline|
+ delete_count += conn.pipelined do |pipeline|
subset.each { |entry| pipeline.del(entry) }
end.sum
end
@@ -58,7 +58,7 @@ def patched_read_multi_mget(*names)
def pipeline_mget(conn, keys)
keys.each_slice(pipeline_batch_size).flat_map do |subset|
- Gitlab::Redis::CrossSlot::Pipeline.new(conn).pipelined do |p|
+ conn.pipelined do |p|
subset.each { |key| p.get(key) }
end
end
diff --git a/lib/gitlab/patch/redis_cluster.rb b/lib/gitlab/patch/redis_cluster.rb
deleted file mode 100644
index 145ce35a31736165daba94c40bc2597debdb0a5f..0000000000000000000000000000000000000000
--- a/lib/gitlab/patch/redis_cluster.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-# Patch to expose `find_node_key` method for cross-slot pipelining
-# In redis v5.0.x, cross-slot pipelining is implemented via redis-cluster-client.
-# This patch should be removed since there is no need for it.
-# Gitlab::Redis::CrossSlot and its usage should be removed as well.
-if Gem::Version.new(Redis::VERSION) != Gem::Version.new('4.8.0')
- raise 'New version of redis detected, please remove or update this patch'
-end
-
-module Gitlab
- module Patch
- module RedisCluster
- # _find_node_key exposes a private function of the same name in Redis::Cluster.
- # See https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/cluster.rb#L282
- def _find_node_key(command)
- find_node_key(command)
- end
- end
- end
-end
diff --git a/lib/gitlab/patch/redis_store_factory.rb b/lib/gitlab/patch/redis_store_factory.rb
new file mode 100644
index 0000000000000000000000000000000000000000..81636ed665dc5e62662e08d0889f5eb7db2e009e
--- /dev/null
+++ b/lib/gitlab/patch/redis_store_factory.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Patch
+ module RedisStoreFactory
+ def create
+ # rubocop:disable Gitlab/ModuleWithInstanceVariables -- patched code references @options in redis-store
+ opt = @options
+ # rubocop:enable Gitlab/ModuleWithInstanceVariables
+ return Gitlab::Redis::ClusterStore.new(opt) if opt[:nodes]
+
+ super
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/patch/slot_loader.rb b/lib/gitlab/patch/slot_loader.rb
deleted file mode 100644
index e302d844078d7d2b9a4d8195ca14dc9b9a486992..0000000000000000000000000000000000000000
--- a/lib/gitlab/patch/slot_loader.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Patch
- module SlotLoader
- extend ActiveSupport::Concern
-
- class_methods do
- # Shuffle the node list to spread out initial connection creation amongst all nodes
- #
- # The input is a Redis::Cluster::Node instance which is an Enumerable.
- # `super` receives an Array of Redis::Client instead of a Redis::Cluster::Node
- def load(nodes)
- super(nodes.to_a.shuffle)
- end
- end
- end
- end
-end
diff --git a/lib/gitlab/redis/cluster_store.rb b/lib/gitlab/redis/cluster_store.rb
new file mode 100644
index 0000000000000000000000000000000000000000..d660c782d4d0199a889316560ac0742786e09580
--- /dev/null
+++ b/lib/gitlab/redis/cluster_store.rb
@@ -0,0 +1,81 @@
+# frozen_string_literal: true
+
+require 'redis-clustering'
+require 'redis/store/ttl'
+require 'redis/store/interface'
+require 'redis/store/namespace'
+require 'redis/store/serialization'
+
+module Gitlab
+ module Redis
+ class ClusterStore < ::Redis::Cluster
+ include ::Redis::Store::Interface
+
+ def initialize(options = {})
+ orig_options = options.dup
+
+ @serializer = orig_options.key?(:serializer) ? orig_options.delete(:serializer) : Marshal
+
+ unless orig_options[:marshalling].nil?
+ # `marshalling` only used here, might not be supported in `super`
+ @serializer = orig_options.delete(:marshalling) ? Marshal : nil
+ end
+
+ _remove_unsupported_options(options)
+ super(options)
+
+ _extend_marshalling
+ _extend_namespace orig_options
+ end
+
+ # copies ::Redis::Store::Ttl implementation in a redis-v5 compatible manner
+ def set(key, value, options = nil)
+ ttl = get_ttl(options)
+ if ttl
+ setex(key, ttl.to_i, value, raw: true)
+ else
+ super(key, value)
+ end
+ end
+
+ # copies ::Redis::Store::Ttl implementation in a redis-v5 compatible manner
+ def setnx(key, value, options = nil)
+ ttl = get_ttl(options)
+ if ttl
+ multi do |m|
+ m.setnx(key, value)
+ m.expire(key, ttl)
+ end
+ else
+ super(key, value)
+ end
+ end
+
+ private
+
+ def get_ttl(options)
+ # https://github.com/redis-store/redis-store/blob/v1.10.0/lib/redis/store/ttl.rb#L37
+ options[:expire_after] || options[:expires_in] || options[:expire_in] if options
+ end
+
+ def _remove_unsupported_options(options)
+ # Unsupported keywords should be removed to avoid errors
+ # https://github.com/redis-rb/redis-client/blob/v0.13.0/lib/redis_client/config.rb#L21
+ options.delete(:raw)
+ options.delete(:serializer)
+ options.delete(:marshalling)
+ options.delete(:namespace)
+ options.delete(:scheme)
+ end
+
+ def _extend_marshalling
+ extend ::Redis::Store::Serialization unless @serializer.nil?
+ end
+
+ def _extend_namespace(options)
+ @namespace = options[:namespace]
+ extend ::Redis::Store::Namespace
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/redis/cluster_util.rb b/lib/gitlab/redis/cluster_util.rb
index 9e307940de3d17141e18e985226cf3e50661982e..99f337749d0d8bdf9970c1c5cf9470fc9cb57b14 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.respond_to?(:_client) && obj._client.is_a?(::Redis::Cluster)
+ obj.is_a?(::Redis::Cluster)
end
end
def batch_unlink(keys, redis)
expired_count = 0
keys.each_slice(1000) do |subset|
- expired_count += Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ expired_count += redis.pipelined do |pipeline|
subset.each { |key| pipeline.unlink(key) }
end.sum
end
@@ -30,7 +30,7 @@ def batch_unlink(keys, redis)
# Redis cluster alternative to mget
def batch_get(keys, redis)
keys.each_slice(1000).flat_map do |subset|
- Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
+ redis.pipelined do |pipeline|
subset.map { |key| pipeline.get(key) }
end
end
diff --git a/lib/gitlab/redis/command_builder.rb b/lib/gitlab/redis/command_builder.rb
new file mode 100644
index 0000000000000000000000000000000000000000..99d26760b3d3786edc0fc06ce951843271b827c7
--- /dev/null
+++ b/lib/gitlab/redis/command_builder.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Redis
+ module CommandBuilder
+ extend self
+
+ # Ref: https://github.com/redis-rb/redis-client/blob/v0.19.1/lib/redis_client/command_builder.rb
+ # we modify the command builder to convert nil to strings as this behaviour was present in
+ # https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/connection/command_helper.rb#L20
+ #
+ # Note that we only adopt the Ruby3.x-compatible logic in .generate.
+ # Symbol.method_defined?(:name) is true in Ruby 3
+ def generate(args, kwargs = nil)
+ command = args.flat_map do |element|
+ case element
+ when Hash
+ element.flatten
+ else
+ element
+ end
+ end
+
+ kwargs&.each do |key, value|
+ if value
+ if value == true
+ command << key.name
+ else
+ command << key.name << value
+ end
+ end
+ end
+
+ command.map! do |element|
+ case element
+ when String
+ element
+ when Symbol
+ element.name
+ when Integer, Float, NilClass
+ element.to_s
+ else
+ raise TypeError, "Unsupported command argument type: #{element.class}"
+ end
+ end
+
+ raise ArgumentError, "can't issue an empty redis command" if command.empty?
+
+ command
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/redis/cross_slot.rb b/lib/gitlab/redis/cross_slot.rb
deleted file mode 100644
index e5aa6d9ce7246d36db668e54c71c25f118860589..0000000000000000000000000000000000000000
--- a/lib/gitlab/redis/cross_slot.rb
+++ /dev/null
@@ -1,141 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
- module Redis
- module CrossSlot
- class Router
- attr_reader :node_mapping, :futures, :node_sequence, :cmd_queue
-
- delegate :respond_to_missing?, to: :@redis
-
- # This map contains redis-rb methods which does not map directly
- # to a standard Redis command. It is used transform unsupported commands to standard commands
- # to find the node key for unsupported commands.
- #
- # Redis::Cluster::Command only contains details of commands which the Redis Server
- # returns. Hence, commands like mapped_hmget and hscan_each internally will call the
- # base command, hmget and hscan respectively.
- #
- # See https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/cluster/command.rb
- UNSUPPORTED_CMD_MAPPING = {
- # Internally, redis-rb calls the supported Redis command and transforms the output.
- # See https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/commands/hashes.rb#L104
- mapped_hmget: :hmget
- }.freeze
-
- # Initializes the CrossSlot::Router
- # @param {::Redis}
- def initialize(redis)
- @redis = redis
- @node_mapping = {}
- @futures = {}
- @node_sequence = []
- @cmd_queue = []
- end
-
- # For now we intercept every redis.call and return a Gitlab-Future object.
- # This method groups every commands to a node for fan-out. Commands are grouped using the first key.
- #
- # rubocop:disable Style/MissingRespondToMissing
- def method_missing(cmd, *args, **kwargs, &blk)
- # Note that we can re-map the command without affecting execution as it is
- # solely for finding the node key. The original cmd will be executed.
- node = @redis._client._find_node_key([UNSUPPORTED_CMD_MAPPING.fetch(cmd, cmd)] + args)
-
- @node_mapping[node] ||= []
- @futures[node] ||= []
-
- @node_sequence << node
- @node_mapping[node] << [cmd, args, kwargs || {}, blk]
- f = Future.new
- @futures[node] << f
- @cmd_queue << [f, cmd, args, kwargs || {}, blk]
- f
- end
- # rubocop:enable Style/MissingRespondToMissing
- end
-
- # Wraps over redis-rb's Future in
- # https://github.com/redis/redis-rb/blob/v4.8.0/lib/redis/pipeline.rb#L244
- class Future
- def set(future, is_val = false)
- @redis_future = future
- @is_val = is_val
- end
-
- def value
- return @redis_val if @is_val
-
- @redis_future.value
- end
- end
-
- # Pipeline allows cross-slot pipelined to be called. The fan-out logic is implemented in
- # https://github.com/redis-rb/redis-cluster-client/blob/master/lib/redis_client/cluster/pipeline.rb
- # which is available in redis-rb v5.0.
- #
- # This file can be deprecated after redis-rb v4.8.0 is upgraded to v5.0
- class Pipeline
- # Initializes the CrossSlot::Pipeline
- # @param {::Redis}
- def initialize(redis)
- @redis = redis
- end
-
- # pipelined is used in place of ::Redis `.pipelined` when running in a cluster context
- # where cross-slot operations may happen.
- def pipelined(&block)
- # Directly call .pipelined and defer the pipeline execution to MultiStore.
- # MultiStore could wrap over 0, 1, or 2 Redis Cluster clients, handling it here
- # will not work for 2 clients since the key-slot topology can differ.
- if use_cross_slot_pipelining?
- router = Router.new(@redis)
- yield router
- execute_commands(router)
- else
- # use redis-rb's pipelined method
- @redis.pipelined(&block)
- end
- end
-
- private
-
- def use_cross_slot_pipelining?
- !@redis.instance_of?(::Gitlab::Redis::MultiStore) && @redis._client.instance_of?(::Redis::Cluster)
- end
-
- def execute_commands(router)
- router.node_mapping.each do |node_key, commands|
- # TODO possibly use Threads to speed up but for now `n` is 3-5 which is small.
- @redis.pipelined do |p|
- commands.each_with_index do |command, idx|
- future = router.futures[node_key][idx]
- cmd, args, kwargs, blk = command
- future.set(p.public_send(cmd, *args, **kwargs, &blk)) # rubocop:disable GitlabSecurity/PublicSend
- end
- end
- end
-
- router.node_sequence.map do |node_key|
- router.futures[node_key].shift.value
- end
- rescue ::Redis::CommandError => err
- if err.message.start_with?('MOVED', 'ASK')
- Gitlab::ErrorTracking.log_exception(err)
- return execute_commands_sequentially(router)
- end
-
- raise
- end
-
- def execute_commands_sequentially(router)
- router.cmd_queue.map do |command|
- future, cmd, args, kwargs, blk = command
- future.set(@redis.public_send(cmd, *args, **kwargs, &blk), true) # rubocop:disable GitlabSecurity/PublicSend
- future.value
- end
- end
- end
- end
- end
-end
diff --git a/lib/gitlab/redis/multi_store.rb b/lib/gitlab/redis/multi_store.rb
index e6262f7f61b516ec17e3f25316d47c8d6d97d42b..476077cb96f0dcbc248022b5db9abdb0b7183dbf 100644
--- a/lib/gitlab/redis/multi_store.rb
+++ b/lib/gitlab/redis/multi_store.rb
@@ -432,16 +432,6 @@ 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|
@@ -462,7 +452,7 @@ def with_instance(instance, *params)
end
def redis_store?(pool)
- pool.with { |c| c.instance_of?(Gitlab::Redis::MultiStore) || c.is_a?(::Redis) }
+ pool.with { |c| c.instance_of?(Gitlab::Redis::MultiStore) || c.is_a?(::Redis) || c.is_a?(::Redis::Cluster) }
end
def validate_stores!
diff --git a/lib/gitlab/redis/wrapper.rb b/lib/gitlab/redis/wrapper.rb
index a106ed3884ef34b3a7a469525018ddaaa8fa5db9..f7a7e703d90e7de08aca82ba94c3bd06dc916566 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, :redis_client_params, to: :new
+ delegate :params, :url, :store, :encrypted_secrets, to: :new
def with
pool.with { |redis| yield redis }
@@ -90,7 +90,17 @@ def instrumentation_class
end
def redis
- ::Redis.new(params)
+ init_redis(params)
+ end
+
+ private
+
+ def init_redis(config)
+ if config[:nodes].present?
+ ::Redis::Cluster.new(config.merge({ concurrency: { model: :none } }))
+ else
+ ::Redis.new(config)
+ end
end
end
@@ -99,13 +109,8 @@ 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
@@ -114,14 +119,14 @@ def redis_client_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
@@ -134,7 +139,7 @@ def url
end
def db
- redis_store_options[:db]
+ redis_store_options[:db] || 0
end
def sentinels
@@ -156,7 +161,7 @@ def sentinels?
end
def store(extras = {})
- ::Redis::Store::Factory.create(redis_store_options.merge(extras))
+ ::Redis::Store::Factory.create(params.merge(extras))
end
def encrypted_secrets
@@ -182,7 +187,11 @@ def redis_store_options
final_config = parse_extra_config(decrypted_config)
result = if final_config[:cluster].present?
- final_config[:db] = 0 # Redis Cluster only supports db 0
+ final_config[:cluster] = final_config[:cluster].map do |node|
+ next node unless node.is_a?(String)
+
+ ::Redis::Store::Factory.extract_host_options_from_uri(node)
+ end
final_config
else
parse_redis_url(final_config)
diff --git a/lib/gitlab/repository_hash_cache.rb b/lib/gitlab/repository_hash_cache.rb
index fab0e9e09e8dae73ce14061908b5b36152085896..6da5556833f2ecd81d6c8e7342214a841bc9ae09 100644
--- a/lib/gitlab/repository_hash_cache.rb
+++ b/lib/gitlab/repository_hash_cache.rb
@@ -86,6 +86,8 @@ 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 eb73a0a3d31eba41e0f9d26c579964b977219871..967a139e7fb3c1e109f6aa6352de4c352f8c4ef1 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)
+ multi.sismember(full_key, value.to_s)
multi.exists?(full_key) # rubocop:disable CodeReuse/ActiveRecord
end
end
diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb
index 883e1ba0558dd65b82852fffb0e0ff7995b77048..c40f125db40d4230a12f063ea7f5c7ddeadcb58a 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 || '', location]
+ argv += [connection_name, diff ? diff.to_f : '', location]
end
with_redis { |r| r.eval(UPDATE_WAL_COOKIE_SCRIPT, keys: [cookie_key], argv: argv) }
diff --git a/spec/controllers/concerns/product_analytics_tracking_spec.rb b/spec/controllers/concerns/product_analytics_tracking_spec.rb
index 7b48782be989e438840e02b4b4ca5532a8c14c29..247be5429a1df4076f8e033eb293ea861c1eb00e 100644
--- a/spec/controllers/concerns/product_analytics_tracking_spec.rb
+++ b/spec/controllers/concerns/product_analytics_tracking_spec.rb
@@ -109,7 +109,8 @@ def expect_no_internal_tracking
end
it 'tracks total Redis counters' do
- expect(Gitlab::Usage::Metrics::Instrumentations::TotalCountMetric).to receive(:redis_key).twice # total and 7d
+ expect(Gitlab::Usage::Metrics::Instrumentations::TotalCountMetric).to receive(:redis_key)
+ .twice.and_call_original # total and 7d
get :index
end
diff --git a/spec/fixtures/config/redis_new_format_host_standalone.yml b/spec/fixtures/config/redis_new_format_host_standalone.yml
new file mode 100644
index 0000000000000000000000000000000000000000..f5836586247389bc74ca40cab1a21d1284671fac
--- /dev/null
+++ b/spec/fixtures/config/redis_new_format_host_standalone.yml
@@ -0,0 +1,8 @@
+# redis://[:password@]host[:port][/db-number][?option=value]
+# more details: http://www.iana.org/assignments/uri-schemes/prov/redis
+development:
+ url: redis://:mynewpassword@development-host:6379/99
+test:
+ url: redis://:mynewpassword@test-host:6379/99
+production:
+ url: redis://:mynewpassword@production-host:6379/99
diff --git a/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb b/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb
index cf82fd751ddea57ebd694267227670f136e5d4de..d89ad21ca82bd57574854736b521842cd727bb45 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('unix:///home/localuser/redis/redis.socket/0')
+ expect(sub.connection[:id]).to eq('/home/localuser/redis/redis.socket/0')
expect(ActionCable.server.config.cable[:id]).to be_nil
end
end
diff --git a/spec/initializers/session_store_spec.rb b/spec/initializers/session_store_spec.rb
index c9333d022dd31a6450b2bdc7582cb36f4492cc56..bd943830f6e879315a9bb38d4530096c185e117f 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_store: kind_of(::Redis::Store)))
+ expect(subject).to receive(:session_store).with(:redis_store, a_hash_including(redis_server: Gitlab::Redis::Sessions.params.merge(namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE)))
load_session_store
end
diff --git a/spec/lib/click_house/migration_support/exclusive_lock_spec.rb b/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
index 5176cc752666addf7eaef212f137ee18d22b784b..17e58a4ddef60f6df3b98eec9198ead3717cf8ce 100644
--- a/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
+++ b/spec/lib/click_house/migration_support/exclusive_lock_spec.rb
@@ -5,6 +5,8 @@
RSpec.describe ClickHouse::MigrationSupport::ExclusiveLock, feature_category: :database do
include ExclusiveLeaseHelpers
+ let(:worker_id) { 1 }
+
let(:worker_class) do
# This worker will be active longer than the ClickHouse worker TTL
Class.new do
@@ -81,7 +83,7 @@ def migration
end
around do |example|
- described_class.register_running_worker(worker_class, anything) do
+ described_class.register_running_worker(worker_class, worker_id) do
example.run
end
end
diff --git a/spec/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl_spec.rb b/spec/lib/gitlab/background_migration/redis/backfill_project_pipeline_status_ttl_spec.rb
index c52d1b4c9f2a593f45bf90b3517ef0d24b312504..a55ae5711a8d040f39e57d0ce884a8112cddb8a0 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) { ::Redis.new(::Gitlab::Redis::Cache.params) }
+ let(:redis) { ::Gitlab::Redis::Cache.redis }
let(:keys) { ["cache:gitlab:project:1:pipeline_status", "cache:gitlab:project:2:pipeline_status"] }
let(:invalid_keys) { ["cache:gitlab:project:pipeline_status:1", "cache:gitlab:project:pipeline_status:2"] }
diff --git a/spec/lib/gitlab/database/migrations/runner_backoff/communicator_spec.rb b/spec/lib/gitlab/database/migrations/runner_backoff/communicator_spec.rb
index cfc3fb398e24eaa82ee130678c84389444e9dd80..9f1ade529b1822dac2f9b04fb77c7ba0f8d41eac 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 30981e4bd7d1185f7bbd12110319e425f5448804..8068be798d392fed31a05fe045d4a8788acf0368 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) }
+ found_key = Gitlab::Redis::Cache.with { |r| r.get(full_key).force_encoding("UTF-8") }
expect(described_class.gzip_decompress(found_key)).to eq(value.to_json)
end
diff --git a/spec/lib/gitlab/external_authorization/cache_spec.rb b/spec/lib/gitlab/external_authorization/cache_spec.rb
index 186bf7d7ec15f1e49448ca556421a5f9191814bc..3dc23422e4edc03dd86ded7a406b7deb291a0d8a 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)
+ set_in_redis(:access, false.to_s)
set_in_redis(:reason, 'Access denied for now')
- set_in_redis(:refreshed_at, Time.now)
+ set_in_redis(:refreshed_at, Time.now.to_s)
access, reason, refreshed_at = cache.load
diff --git a/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb b/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
index a8bded69696e656992f22dcbeffb2141c595e81c..2eb77add2ed6997542fa009acf2c708949d624d8 100644
--- a/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
+++ b/spec/lib/gitlab/instrumentation/redis_client_middleware_spec.rb
@@ -9,10 +9,9 @@
include RedisHelpers
let_it_be(:redis_store_class) { define_helper_redis_store_class }
- let_it_be(:redis_client) { RedisClient.new(redis_store_class.redis_client_params) }
before do
- redis_client.call("flushdb")
+ redis_store_class.with(&:flushdb)
end
describe 'read and write' do
@@ -24,27 +23,30 @@
# The response is 1001, so 4 bytes. Exercise counting an integer reply.
[[:set, 'foobar', 1000]] | [:incr, 'foobar'] | (4 + 6) | 4
- # Exercise counting empty multi bulk reply. Returns an empty hash `{}`
- [] | [:hgetall, 'foobar'] | (7 + 6) | 2
+ # Exercise counting empty multi bulk reply.
+ [] | [:hgetall, 'foobar'] | (7 + 6) | 0
# Hgetall response length is combined length of keys and values in the
# hash. Exercises counting of a multi bulk reply
- # Returns `{"field"=>"hello world"}`, 5 for field, 11 for hello world, 8 for {, }, 4 "s, =, >
- [[:hset, 'myhash', 'field', 'hello world']] | [:hgetall, 'myhash'] | (7 + 6) | (5 + 11 + 8)
+ [[:hset, 'myhash', 'field', 'hello world']] | [:hgetall, 'myhash'] | (7 + 6) | (5 + 11)
# Exercise counting of a bulk reply
[[:set, 'foo', 'bar' * 100]] | [:get, 'foo'] | (3 + 3) | (3 * 100)
- # Nested array response: [['foo', 0.0], ['bar', 1.0]]. Returns scores as float.
+ # Nested array response: [['foo', 0], ['bar', 1.1000000000000001]] due to Redis precision
+ # See https://github.com/redis/redis/issues/1499
[[:zadd, 'myset', 0, 'foo'],
- [:zadd, 'myset', 1, 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | (6 + 5 + 1 + 2 + 10) | (3 + 3 + 3 + 3)
+ [:zadd, 'myset', 1.1,
+ 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | (6 + 5 + 1 + 2 + 10) | (3 + 1 + 3 + 18)
end
with_them do
it 'counts bytes read and written' do
- setup.each { |cmd| redis_client.call(*cmd) }
- RequestStore.clear!
- redis_client.call(*command)
+ redis_store_class.with do |redis|
+ setup.each { |cmd| redis.call(cmd) }
+ RequestStore.clear!
+ redis.call(command)
+ end
expect(Gitlab::Instrumentation::Redis.read_bytes).to eq(expect_read)
expect(Gitlab::Instrumentation::Redis.write_bytes).to eq(expect_write)
@@ -58,35 +60,48 @@
it 'counts successful requests' do
expect(instrumentation_class).to receive(:instance_count_request).with(1).and_call_original
- redis_client.call(:get, 'foobar')
+ redis_store_class.with { |redis| redis.call(:get, 'foobar') }
end
it 'counts successful pipelined requests' do
expect(instrumentation_class).to receive(:instance_count_request).with(2).and_call_original
expect(instrumentation_class).to receive(:instance_count_pipelined_request).with(2).and_call_original
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:get, '{foobar}baz')
+ end
end
end
context 'when encountering exceptions' do
- before do
- allow(redis_client.instance_variable_get(:@raw_connection)).to receive(:call).and_raise(
- RedisClient::Error)
+ where(:case_name, :exception, :exception_counter) do
+ 'generic exception' | Redis::CommandError.new | :instance_count_exception
+ 'moved redirection' | Redis::CommandError.new("MOVED 123 127.0.0.1:6380") | :instance_count_cluster_redirection
+ 'ask redirection' | Redis::CommandError.new("ASK 123 127.0.0.1:6380") | :instance_count_cluster_redirection
end
- it 'counts exception' do
- expect(instrumentation_class).to receive(:instance_count_exception)
- .with(instance_of(RedisClient::Error)).and_call_original
- expect(instrumentation_class).to receive(:log_exception)
- .with(instance_of(RedisClient::Error)).and_call_original
- expect(instrumentation_class).to receive(:instance_count_request).and_call_original
+ with_them do
+ before do
+ redis_store_class.with do |redis|
+ # We need to go 1 layer deeper to stub _client as we monkey-patch Redis::Client
+ # with the interceptor. Stubbing `redis` will skip the instrumentation_class.
+ allow(redis._client.instance_variable_get(:@raw_connection)).to receive(:call).and_raise(exception)
+ end
+ end
+
+ 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_client.call(:auth, 'foo', 'bar')
- end.to raise_error(RedisClient::Error)
+ expect do
+ redis_store_class.with { |redis| redis.call(:auth, 'foo', 'bar') }
+ end.to raise_exception(Redis::CommandError)
+ end
end
end
@@ -99,7 +114,7 @@
expect(instrumentation_class).to receive(:increment_cross_slot_request_count).and_call_original
expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
- redis_client.call(:mget, 'foo', 'bar')
+ redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
end
it 'does not count allowed cross-slot requests' do
@@ -107,7 +122,7 @@
expect(instrumentation_class).to receive(:increment_allowed_cross_slot_request_count).and_call_original
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_client.call(:mget, 'foo', 'bar')
+ redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
end
end
@@ -116,7 +131,7 @@
expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_client.call(:mget, 'bar')
+ redis_store_class.with { |redis| redis.call(:get, 'bar') }
end
end
@@ -124,7 +139,7 @@
expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
- redis_client.call(:mget, '{foo}bar', '{foo}baz')
+ redis_store_class.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') }
end
end
@@ -135,7 +150,7 @@
it 'still runs cross-slot validation' do
expect do
- redis_client.call('mget', 'foo', 'bar')
+ redis_store_class.with { |redis| redis.mget('foo', 'bar') }
end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
end
end
@@ -157,7 +172,7 @@
expect(instrumentation_class).to receive(:instance_observe_duration).with(a_value > 0)
.and_call_original
- redis_client.call(*command)
+ redis_store_class.with { |redis| redis.call(*command) }
end
end
@@ -166,17 +181,21 @@
expect(instrumentation_class).to receive(:instance_observe_duration).twice.with(a_value > 0)
.and_call_original
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:get, '{foobar}baz')
+ end
end
end
it 'raises error when keys are not from the same slot' do
expect do
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, 'foo')
- pipeline.call(:get, 'bar')
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, 'foo')
+ pipeline.call(:get, 'bar')
+ end
end
end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
end
@@ -200,11 +219,11 @@
with_them do
it 'skips requests we do not want in the apdex' do
- setup.each { |cmd| redis_client.call(*cmd) }
+ setup.each { |cmd| redis_store_class.with { |redis| redis.call(*cmd) } }
expect(instrumentation_class).not_to receive(:instance_observe_duration)
- redis_client.call(*command)
+ redis_store_class.with { |redis| redis.call(*command) }
end
end
@@ -212,10 +231,12 @@
it 'skips requests that have blocking commands' do
expect(instrumentation_class).not_to receive(:instance_observe_duration)
- redis_client.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:rpush, '{foobar}baz', 1)
- pipeline.call(:brpop, '{foobar}baz', 0)
+ redis_store_class.with do |redis|
+ redis.pipelined do |pipeline|
+ pipeline.call(:get, '{foobar}buz')
+ pipeline.call(:rpush, '{foobar}baz', 1)
+ pipeline.call(:brpop, '{foobar}baz', 0)
+ end
end
end
end
diff --git a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
deleted file mode 100644
index 2a160a9d31662644ce4211fc0f4e2b43072be7ae..0000000000000000000000000000000000000000
--- a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb
+++ /dev/null
@@ -1,261 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-require 'rspec-parameterized'
-require 'support/helpers/rails_helpers'
-
-RSpec.describe Gitlab::Instrumentation::RedisInterceptor, :request_store, feature_category: :scalability do
- using RSpec::Parameterized::TableSyntax
- include RedisHelpers
-
- let_it_be(:redis_store_class) { define_helper_redis_store_class }
-
- before do
- redis_store_class.with(&:flushdb)
- end
-
- describe 'read and write' do
- where(:setup, :command, :expect_write, :expect_read) do
- # The response is 'OK', the request size is the combined size of array
- # elements. Exercise counting of a status reply.
- [] | [:set, 'foo', 'bar'] | 3 + 3 + 3 | 2
-
- # The response is 1001, so 4 bytes. Exercise counting an integer reply.
- [[:set, 'foobar', 1000]] | [:incr, 'foobar'] | 4 + 6 | 4
-
- # Exercise counting empty multi bulk reply
- [] | [:hgetall, 'foobar'] | 7 + 6 | 0
-
- # Hgetall response length is combined length of keys and values in the
- # hash. Exercises counting of a multi bulk reply
- [[:hset, 'myhash', 'field', 'hello world']] | [:hgetall, 'myhash'] | 7 + 6 | 5 + 11
-
- # Exercise counting of a bulk reply
- [[:set, 'foo', 'bar' * 100]] | [:get, 'foo'] | 3 + 3 | 3 * 100
-
- # Nested array response: [['foo', 0], ['bar', 1]]
- [[:zadd, 'myset', 0, 'foo'], [:zadd, 'myset', 1, 'bar']] | [:zrange, 'myset', 0, -1, 'withscores'] | 6 + 5 + 1 + 2 + 10 | 3 + 1 + 3 + 1
- end
-
- with_them do
- it 'counts bytes read and written' do
- redis_store_class.with do |redis|
- setup.each { |cmd| redis.call(cmd) }
- RequestStore.clear!
- redis.call(command)
- end
-
- expect(Gitlab::Instrumentation::Redis.read_bytes).to eq(expect_read)
- expect(Gitlab::Instrumentation::Redis.write_bytes).to eq(expect_write)
- end
- end
- end
-
- describe 'counting' do
- let(:instrumentation_class) { redis_store_class.instrumentation_class }
-
- it 'counts successful requests' do
- expect(instrumentation_class).to receive(:instance_count_request).with(1).and_call_original
-
- redis_store_class.with { |redis| redis.call(:get, 'foobar') }
- end
-
- it 'counts successful pipelined requests' do
- expect(instrumentation_class).to receive(:instance_count_request).with(2).and_call_original
- expect(instrumentation_class).to receive(:instance_count_pipelined_request).with(2).and_call_original
-
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
- end
- end
- end
-
- context 'when encountering connection exceptions within process' do
- before do
- redis_store_class.with do |redis|
- allow(redis._client).to receive(:write).and_call_original
- end
- end
-
- it 'counts connection exceptions' do
- redis_store_class.with do |redis|
- expect(redis._client).to receive(:write).with([:get, 'foobar']).and_raise(::Redis::ConnectionError)
- end
-
- expect(instrumentation_class).to receive(:instance_count_connection_exception)
- .with(instance_of(Redis::ConnectionError)).and_call_original
-
- redis_store_class.with { |redis| redis.call(:get, 'foobar') }
- end
- end
-
- context 'when encountering exceptions' do
- where(:case_name, :exception, :exception_counter) do
- 'generic exception' | Redis::CommandError | :instance_count_exception
- 'moved redirection' | Redis::CommandError.new("MOVED 123 127.0.0.1:6380") | :instance_count_cluster_redirection
- 'ask redirection' | Redis::CommandError.new("ASK 123 127.0.0.1:6380") | :instance_count_cluster_redirection
- end
-
- with_them do
- before do
- redis_store_class.with do |redis|
- # We need to go 1 layer deeper to stub _client as we monkey-patch Redis::Client
- # with the interceptor. Stubbing `redis` will skip the instrumentation_class.
- allow(redis._client).to receive(:process).and_raise(exception)
- end
- end
-
- it 'counts exception' do
- expect(instrumentation_class).to receive(exception_counter)
- .with(instance_of(Redis::CommandError)).and_call_original
- expect(instrumentation_class).to receive(:log_exception)
- .with(instance_of(Redis::CommandError)).and_call_original
- expect(instrumentation_class).to receive(:instance_count_request).and_call_original
-
- expect do
- redis_store_class.with { |redis| redis.call(:auth, 'foo', 'bar') }
- end.to raise_exception(Redis::CommandError)
-
- expect(Thread.current[:redis_client_error_count]).to eq(0)
- end
- end
- end
-
- context 'in production environment' do
- before do
- stub_rails_env('production') # to avoid raising CrossSlotError
- end
-
- it 'counts disallowed cross-slot requests' do
- expect(instrumentation_class).to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
- end
-
- it 'does not count allowed cross-slot requests' do
- expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_store_class.with { |redis| redis.call(:mget, 'foo', 'bar') }
- end
- end
-
- it 'does not count allowed non-cross-slot requests' do
- expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_store_class.with { |redis| redis.call(:get, 'bar') }
- end
- end
-
- it 'skips count for non-cross-slot requests' do
- expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original
- expect(instrumentation_class).not_to receive(:increment_allowed_cross_slot_request_count).and_call_original
-
- redis_store_class.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') }
- end
- end
-
- context 'without active RequestStore' do
- before do
- ::RequestStore.end!
- end
-
- it 'still runs cross-slot validation' do
- expect do
- redis_store_class.with { |redis| redis.mget('foo', 'bar') }
- end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
- end
- end
- end
-
- describe 'latency' do
- let(:instrumentation_class) { redis_store_class.instrumentation_class }
-
- describe 'commands in the apdex' do
- where(:command) do
- [
- [[:get, 'foobar']],
- [%w[GET foobar]]
- ]
- end
-
- with_them do
- it 'measures requests we want in the apdex' do
- expect(instrumentation_class).to receive(:instance_observe_duration).with(a_value > 0)
- .and_call_original
-
- redis_store_class.with { |redis| redis.call(*command) }
- end
- end
-
- context 'with pipelined commands' do
- it 'measures requests that do not have blocking commands' do
- expect(instrumentation_class).to receive(:instance_observe_duration).twice.with(a_value > 0)
- .and_call_original
-
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:get, '{foobar}baz')
- end
- end
- end
-
- it 'raises error when keys are not from the same slot' do
- expect do
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, 'foo')
- pipeline.call(:get, 'bar')
- end
- end
- end.to raise_error(instance_of(Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError))
- end
- end
- end
-
- describe 'commands not in the apdex' do
- where(:setup, :command) do
- [['rpush', 'foobar', 1]] | ['brpop', 'foobar', 0]
- [['rpush', 'foobar', 1]] | ['blpop', 'foobar', 0]
- [['rpush', '{abc}foobar', 1]] | ['brpoplpush', '{abc}foobar', '{abc}bazqux', 0]
- [['rpush', '{abc}foobar', 1]] | ['brpoplpush', '{abc}foobar', '{abc}bazqux', 0]
- [['zadd', 'foobar', 1, 'a']] | ['bzpopmin', 'foobar', 0]
- [['zadd', 'foobar', 1, 'a']] | ['bzpopmax', 'foobar', 0]
- [['xadd', 'mystream', 1, 'myfield', 'mydata']] | ['xread', 'block', 1, 'streams', 'mystream', '0-0']
- [['xadd', 'foobar', 1, 'myfield', 'mydata'], ['xgroup', 'create', 'foobar', 'mygroup', 0]] | ['xreadgroup', 'group', 'mygroup', 'myconsumer', 'block', 1, 'streams', 'foobar', '0-0']
- [] | ['command']
- end
-
- with_them do
- it 'skips requests we do not want in the apdex' do
- redis_store_class.with { |redis| setup.each { |cmd| redis.call(*cmd) } }
-
- expect(instrumentation_class).not_to receive(:instance_observe_duration)
-
- redis_store_class.with { |redis| redis.call(*command) }
- end
- end
-
- context 'with pipelined commands' do
- it 'skips requests that have blocking commands' do
- expect(instrumentation_class).not_to receive(:instance_observe_duration)
-
- redis_store_class.with do |redis|
- redis.pipelined do |pipeline|
- pipeline.call(:get, '{foobar}buz')
- pipeline.call(:rpush, '{foobar}baz', 1)
- pipeline.call(:brpop, '{foobar}baz', 0)
- end
- end
- end
- end
- end
- end
-end
diff --git a/spec/lib/gitlab/patch/node_loader_spec.rb b/spec/lib/gitlab/patch/node_loader_spec.rb
deleted file mode 100644
index 000083fc6d06ba65665667fce0eb47e9398c6ee6..0000000000000000000000000000000000000000
--- a/spec/lib/gitlab/patch/node_loader_spec.rb
+++ /dev/null
@@ -1,80 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Patch::NodeLoader, feature_category: :redis do
- using RSpec::Parameterized::TableSyntax
-
- describe '#fetch_node_info' do
- let(:redis) { double(:redis) } # rubocop:disable RSpec/VerifiedDoubles
-
- # rubocop:disable Naming/InclusiveLanguage
- where(:case_name, :args, :value) do
- [
- [
- 'when only ip address is present',
- "07c37df 127.0.0.1:30004@31004 slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002 master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005 slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001 myself,master - 0 0 1 connected 0-5460",
- {
- '127.0.0.1:30004' => 'slave', '127.0.0.1:30002' => 'master', '127.0.0.1:30003' => 'master',
- '127.0.0.1:30005' => 'slave', '127.0.0.1:30006' => 'slave', '127.0.0.1:30001' => 'master'
- }
- ],
- [
- 'when hostname is present',
- "07c37df 127.0.0.1:30004@31004,host1 slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002,host2 master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003,host3 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005,host4 slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006,host5 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001,host6 myself,master - 0 0 1 connected 0-5460",
- {
- 'host1:30004' => 'slave', 'host2:30002' => 'master', 'host3:30003' => 'master',
- 'host4:30005' => 'slave', 'host5:30006' => 'slave', 'host6:30001' => 'master'
- }
- ],
- [
- 'when auxiliary fields are present',
- "07c37df 127.0.0.1:30004@31004,,shard-id=69bc slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002,,shard-id=114f master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003,,shard-id=fdb3 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005,,shard-id=114f slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006,,shard-id=fdb3 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001,,shard-id=69bc myself,master - 0 0 1 connected 0-5460",
- {
- '127.0.0.1:30004' => 'slave', '127.0.0.1:30002' => 'master', '127.0.0.1:30003' => 'master',
- '127.0.0.1:30005' => 'slave', '127.0.0.1:30006' => 'slave', '127.0.0.1:30001' => 'master'
- }
- ],
- [
- 'when hostname and auxiliary fields are present',
- "07c37df 127.0.0.1:30004@31004,host1,shard-id=69bc slave e7d1eec 0 1426238317239 4 connected
-67ed2db 127.0.0.1:30002@31002,host2,shard-id=114f master - 0 1426238316232 2 connected 5461-10922
-292f8b3 127.0.0.1:30003@31003,host3,shard-id=fdb3 master - 0 1426238318243 3 connected 10923-16383
-6ec2392 127.0.0.1:30005@31005,host4,shard-id=114f slave 67ed2db 0 1426238316232 5 connected
-824fe11 127.0.0.1:30006@31006,host5,shard-id=fdb3 slave 292f8b3 0 1426238317741 6 connected
-e7d1eec 127.0.0.1:30001@31001,host6,shard-id=69bc myself,master - 0 0 1 connected 0-5460",
- {
- 'host1:30004' => 'slave', 'host2:30002' => 'master', 'host3:30003' => 'master',
- 'host4:30005' => 'slave', 'host5:30006' => 'slave', 'host6:30001' => 'master'
- }
- ]
- ]
- end
- # rubocop:enable Naming/InclusiveLanguage
-
- with_them do
- before do
- allow(redis).to receive(:call).with([:cluster, :nodes]).and_return(args)
- end
-
- it do
- expect(Redis::Cluster::NodeLoader.load_flags([redis])).to eq(value)
- end
- end
- end
-end
diff --git a/spec/lib/gitlab/patch/redis_cache_store_spec.rb b/spec/lib/gitlab/patch/redis_cache_store_spec.rb
index 21c256fdbbee3b36430903a98089d74b944e3ec8..ae9c31e0c513a55c49545be50c3b5a6f7fe96b42 100644
--- a/spec/lib/gitlab/patch/redis_cache_store_spec.rb
+++ b/spec/lib/gitlab/patch/redis_cache_store_spec.rb
@@ -13,6 +13,8 @@
cache.write('{user1}:x', 1)
cache.write('{user1}:y', 2)
cache.write('{user1}:z', 3)
+
+ cache.instance_variable_set(:@pipeline_batch_size, nil)
end
describe '#read_multi_mget' do
@@ -34,7 +36,7 @@
end
context 'when reading large amount of keys' do
- let(:input_size) { 2000 }
+ let(:input_size) { 2100 }
let(:chunk_size) { 1000 }
shared_examples 'read large amount of keys' do
@@ -45,10 +47,11 @@
::Gitlab::Redis::ClusterUtil.cluster?(redis.default_store)
if normal_cluster || multistore_cluster
- expect_next_instances_of(Gitlab::Redis::CrossSlot::Pipeline, 2) do |pipeline|
- obj = instance_double(::Redis)
- expect(pipeline).to receive(:pipelined).and_yield(obj)
- expect(obj).to receive(:get).exactly(chunk_size).times
+ times = (input_size.to_f / chunk_size).ceil
+ expect(redis).to receive(:pipelined).exactly(times).times.and_call_original
+
+ expect_next_instances_of(::Redis::PipelinedConnection, times) do |p|
+ expect(p).to receive(:get).at_most(chunk_size).times
end
else
expect(redis).to receive(:mget).and_call_original
diff --git a/spec/lib/gitlab/patch/redis_client_spec.rb b/spec/lib/gitlab/patch/redis_client_spec.rb
index af094e9e0d25185adc913d8b08d0296b335235af..3e2d46dd19c8c3cbb17992aa278248832b132a89 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.redis_client_params) }
+ let_it_be(:redis_client) { RedisClient.new(redis_store_class.params) }
before do
Thread.current[:redis_client_error_count] = 1
diff --git a/spec/lib/gitlab/patch/redis_store_factory_spec.rb b/spec/lib/gitlab/patch/redis_store_factory_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..be37a044ac154fd8c4dde9ba7005d84dcfe9ec67
--- /dev/null
+++ b/spec/lib/gitlab/patch/redis_store_factory_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Patch::RedisStoreFactory, feature_category: :redis do
+ describe '#create' do
+ let(:params) { { host: 'localhost' } }
+
+ subject(:factory_create) { ::Redis::Store::Factory.create(params) } # rubocop:disable Rails/SaveBang -- redis-store does not implement create!
+
+ context 'when using standalone Redis' do
+ it 'does not create ClusterStore' do
+ expect(Gitlab::Redis::ClusterStore).not_to receive(:new)
+
+ factory_create
+ end
+ end
+
+ context 'when using a Redis Cluster' do
+ let(:params) { { nodes: ["redis://localhost:6001", "redis://localhost:6002"] } }
+
+ it 'creates a ClusterStore' do
+ expect(Gitlab::Redis::ClusterStore).to receive(:new).with(params.merge({ raw: false }))
+
+ factory_create
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/rack_attack/store_spec.rb b/spec/lib/gitlab/rack_attack/store_spec.rb
index 19b3f239d915f8c42c8cbeff428a5447d35fa415..efe6f9382f954e93d0249fe63f75e33920f15e09 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',
- instrumentation_class: Gitlab::Redis::RateLimiting.instrumentation_class
+ custom: { instrumentation_class: Gitlab::Redis::RateLimiting.instrumentation_class }
)
allow(Gitlab::Redis::RateLimiting).to receive(:with).and_yield(broken_redis)
end
diff --git a/spec/lib/gitlab/redis/cluster_store_spec.rb b/spec/lib/gitlab/redis/cluster_store_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..aaeee6156a0d882c536d766fb0f39da734bd6aae
--- /dev/null
+++ b/spec/lib/gitlab/redis/cluster_store_spec.rb
@@ -0,0 +1,116 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# This spec only runs if a Redis Cluster is configured for Gitlab::Redis::Cache.
+# ::Redis::Cluster fetches the cluster details from the server on `initialize` and will raise
+# an error if the cluster is not found.
+#
+# An example would be the following in config/redis.yml assuming gdk is set up with redis-cluster.
+# test:
+# cache
+# cluster:
+# - "redis://127.0.0.1:6003"
+# - "redis://127.0.0.1:6004"
+# - "redis://127.0.0.1:6005"
+RSpec.describe Gitlab::Redis::ClusterStore, :clean_gitlab_redis_cache,
+ feature_category: :redis, if: ::Gitlab::Redis::Cache.params[:nodes] do
+ let(:params) { ::Gitlab::Redis::Cache.params }
+
+ subject(:store) { ::Redis::Store::Factory.create(params) } # rubocop:disable Rails/SaveBang -- not a rails method
+
+ describe '.new' do
+ it 'initialises a cluster store' do
+ expect(store).to be_instance_of(::Gitlab::Redis::ClusterStore)
+ end
+
+ it 'extends Serialization by default' do
+ expect(store.is_a?(::Redis::Store::Serialization)).to eq(true)
+ end
+
+ it 'sets a default serializer when left empty' do
+ expect(store.instance_variable_get(:@serializer)).to eq(Marshal)
+ end
+
+ context 'when serializer field is defined' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(serializer: Class) }
+
+ it 'sets serializer according to the options' do
+ expect(store.instance_variable_get(:@serializer)).to eq(Class)
+ end
+ end
+
+ context 'when marshalling field is defined' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(marshalling: true, serializer: Class) }
+
+ it 'overrides serializer with Marshal' do
+ expect(store.instance_variable_get(:@serializer)).to eq(Marshal)
+ end
+ end
+
+ context 'when marshalling field is false' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(marshalling: false, serializer: Class) }
+
+ it 'overrides serializer with Marshal' do
+ expect(store.instance_variable_get(:@serializer)).to eq(nil)
+ end
+ end
+
+ context 'when namespace is defined' do
+ let(:params) { ::Gitlab::Redis::Cache.params.merge(namespace: 'testing') }
+
+ it 'extends namespace' do
+ expect(store.is_a?(::Redis::Store::Namespace)).to eq(true)
+ end
+
+ it 'write keys with namespace' do
+ store.set('testkey', 1)
+
+ ::Gitlab::Redis::Cache.with do |conn|
+ expect(conn.exists('testing:testkey')).to eq(1)
+ end
+ end
+ end
+ end
+
+ describe '#set' do
+ context 'when ttl is added' do
+ it 'writes the key and sets a ttl' do
+ expect(store.set('test', 1, expire_after: 100)).to eq('OK')
+
+ expect(store.ttl('test')).to be > 95
+ expect(store.get('test')).to eq(1)
+ end
+ end
+
+ context 'when there is no ttl' do
+ it 'sets the key' do
+ expect(store.set('test', 1)).to eq('OK')
+
+ expect(store.get('test')).to eq(1)
+ expect(store.ttl('test')).to eq(-1)
+ end
+ end
+ end
+
+ describe '#setnx' do
+ context 'when ttl is added' do
+ it 'writes the key if not exists and sets a ttl' do
+ expect(store.setnx('test', 1, expire_after: 100)).to eq([true, true])
+ expect(store.ttl('test')).to be > 95
+ expect(store.get('test')).to eq(1)
+ expect(store.setnx('test', 1, expire_after: 100)).to eq([false, true])
+ end
+ end
+
+ context 'when there is no ttl' do
+ it 'writes the key if not exists' do
+ expect(store.setnx('test', 1)).to eq(true)
+ expect(store.setnx('test', 1)).to eq(false)
+
+ expect(store.get('test')).to eq(1)
+ expect(store.ttl('test')).to eq(-1)
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/redis/cluster_util_spec.rb b/spec/lib/gitlab/redis/cluster_util_spec.rb
index a517545014508012798a37abb3df9013367ff9ad..1cd6a450584bb1542fe157724b7aeb1dd95cc9c2 100644
--- a/spec/lib/gitlab/redis/cluster_util_spec.rb
+++ b/spec/lib/gitlab/redis/cluster_util_spec.rb
@@ -5,10 +5,14 @@
RSpec.describe Gitlab::Redis::ClusterUtil, feature_category: :scalability do
using RSpec::Parameterized::TableSyntax
+ let(:router_stub) { instance_double(::RedisClient::Cluster::Router) }
+
+ before do
+ allow(::RedisClient::Cluster::Router).to receive(:new).and_return(router_stub)
+ end
+
describe '.cluster?' do
context 'when MultiStore' do
- let(:redis_cluster) { instance_double(::Redis::Cluster) }
-
where(:pri_store, :sec_store, :expected_val) do
:cluster | :cluster | true
:cluster | :single | true
@@ -17,10 +21,7 @@
end
before do
- # stub all initialiser steps in Redis::Cluster.new to avoid connecting to a Redis Cluster node
- allow(::Redis::Cluster).to receive(:new).and_return(redis_cluster)
- allow(redis_cluster).to receive(:is_a?).with(::Redis::Cluster).and_return(true)
- allow(redis_cluster).to receive(:id).and_return(1)
+ allow(router_stub).to receive(:node_keys).and_return([])
allow(Gitlab::Redis::MultiStore).to receive(:same_redis_store?).and_return(false)
skip_default_enabled_yaml_check
@@ -28,8 +29,8 @@
with_them do
it 'returns expected value' do
- primary_redis = pri_store == :cluster ? ::Redis.new(cluster: ['redis://localhost:6000']) : ::Redis.new
- secondary_redis = sec_store == :cluster ? ::Redis.new(cluster: ['redis://localhost:6000']) : ::Redis.new
+ primary_redis = pri_store == :cluster ? Redis::Cluster.new(nodes: ['redis://localhost:6000']) : Redis.new
+ secondary_redis = sec_store == :cluster ? Redis::Cluster.new(nodes: ['redis://localhost:6000']) : Redis.new
primary_pool = ConnectionPool.new { primary_redis }
secondary_pool = ConnectionPool.new { secondary_redis }
multistore = Gitlab::Redis::MultiStore.new(primary_pool, secondary_pool, 'teststore')
@@ -48,16 +49,8 @@
end
context 'when is Redis::Cluster' do
- let(:redis_cluster) { instance_double(::Redis::Cluster) }
-
- before do
- # stub all initialiser steps in Redis::Cluster.new to avoid connecting to a Redis Cluster node
- allow(::Redis::Cluster).to receive(:new).and_return(redis_cluster)
- allow(redis_cluster).to receive(:is_a?).with(::Redis::Cluster).and_return(true)
- end
-
it 'returns true' do
- expect(described_class.cluster?(::Redis.new(cluster: ['redis://localhost:6000']))).to be_truthy
+ expect(described_class.cluster?(Redis::Cluster.new(nodes: ['redis://localhost:6000']))).to be_truthy
end
end
end
diff --git a/spec/lib/gitlab/redis/command_builder_spec.rb b/spec/lib/gitlab/redis/command_builder_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..7ebb0228d2d7a9fd027b6383870d8c06808f6bcd
--- /dev/null
+++ b/spec/lib/gitlab/redis/command_builder_spec.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# references specs in https://github.com/redis-rb/redis-client/blob/master/test/redis_client/command_builder_test.rb
+# we add `handles nil arguments` to test our own added logic
+RSpec.describe Gitlab::Redis::CommandBuilder, feature_category: :redis do
+ describe '.generate' do
+ def call(*args, **kwargs)
+ described_class.generate(args, kwargs)
+ end
+
+ it 'handles nil arguments' do
+ expect(call("a", nil)).to eq(["a", ""])
+ end
+
+ it 'handles positional arguments' do
+ expect(call("a", "b", "c")).to eq(%w[a b c])
+ end
+
+ it 'handles arrays' do
+ expect(call("a", %w[b c])).to eq(%w[a b c])
+ end
+
+ it 'handles hashes' do
+ expect(call("a", { "b" => "c" })).to eq(%w[a b c])
+ end
+
+ it 'handles symbols' do
+ expect(call(:a, { b: :c }, :d)).to eq(%w[a b c d])
+ end
+
+ it 'handles numerics' do
+ expect(call(1, 2.3)).to eq(["1", "2.3"])
+ end
+
+ it 'handles kwargs booleans' do
+ expect(call(ttl: nil, ex: false, withscores: true)).to eq(["withscores"])
+ end
+
+ it 'handles kwargs values' do
+ expect(call(ttl: 42)).to eq(%w[ttl 42])
+ end
+
+ it 'handles nil kwargs' do
+ expect(call(%i[a b c])).to eq(%w[a b c])
+ end
+
+ it 'raises error on unsupported types' do
+ expect { call(hash: {}) }.to raise_error(TypeError)
+ end
+
+ it 'raises error on empty commands' do
+ expect { call }.to raise_error(ArgumentError)
+ end
+ end
+end
diff --git a/spec/lib/gitlab/redis/cross_slot_spec.rb b/spec/lib/gitlab/redis/cross_slot_spec.rb
deleted file mode 100644
index 4e9830f4110a2bcc6bfb9e2dd8e314725c7071c3..0000000000000000000000000000000000000000
--- a/spec/lib/gitlab/redis/cross_slot_spec.rb
+++ /dev/null
@@ -1,134 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Redis::CrossSlot, feature_category: :redis do
- include RedisHelpers
-
- let_it_be(:redis_store_class) { define_helper_redis_store_class }
-
- before do
- redis_store_class.with(&:flushdb)
- end
-
- describe '.pipelined' do
- context 'when using redis client' do
- before do
- redis_store_class.with { |redis| redis.set('a', 1) }
- end
-
- it 'performs redis-rb pipelined' do
- expect(Gitlab::Redis::CrossSlot::Router).not_to receive(:new)
-
- expect(
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- redis_store_class.with do |redis|
- described_class::Pipeline.new(redis).pipelined do |p|
- p.get('a')
- p.set('b', 1)
- end
- end
- end
- ).to eq(%w[1 OK])
- end
- end
-
- context 'when using with MultiStore' do
- let_it_be(:primary_db) { 1 }
- let_it_be(:secondary_db) { 2 }
- let_it_be(:primary_store) { create_redis_store(redis_store_class.params, db: primary_db, serializer: nil) }
- let_it_be(:secondary_store) { create_redis_store(redis_store_class.params, db: secondary_db, serializer: nil) }
- let_it_be(:primary_pool) { ConnectionPool.new { primary_store } }
- let_it_be(:secondary_pool) { ConnectionPool.new { secondary_store } }
- let_it_be(:multistore) { Gitlab::Redis::MultiStore.new(primary_pool, secondary_pool, 'testing') }
-
- before do
- primary_store.set('a', 1)
- secondary_store.set('a', 1)
- skip_default_enabled_yaml_check
- end
-
- it 'performs multistore pipelined' do
- expect(Gitlab::Redis::CrossSlot::Router).not_to receive(:new)
-
- expect(
- Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
- multistore.with_borrowed_connection do
- described_class::Pipeline.new(multistore).pipelined do |p|
- p.get('a')
- p.set('b', 1)
- end
- end
- end
- ).to eq(%w[1 OK])
- end
- end
-
- context 'when using Redis::Cluster' do
- # Only stub redis client internals since the CI pipeline does not run a Redis Cluster
- let(:redis) { double(:redis) } # rubocop:disable RSpec/VerifiedDoubles
- let(:client) { double(:client) } # rubocop:disable RSpec/VerifiedDoubles
- let(:pipeline) { double(:pipeline) } # rubocop:disable RSpec/VerifiedDoubles
-
- let(:arguments) { %w[a b c d] }
-
- subject do
- described_class::Pipeline.new(redis).pipelined do |p|
- arguments.each { |key| p.get(key) }
- end
- end
-
- before do
- allow(redis).to receive(:_client).and_return(client)
- allow(redis).to receive(:pipelined).and_yield(pipeline)
- allow(client).to receive(:instance_of?).with(::Redis::Cluster).and_return(true)
- end
-
- it 'fan-out and fan-in commands to separate shards' do
- # simulate fan-out to 3 shards with random order
- expect(client).to receive(:_find_node_key).exactly(4).times.and_return(3, 2, 1, 3)
-
- arguments.each do |key|
- f = double('future') # rubocop:disable RSpec/VerifiedDoubles
- expect(pipeline).to receive(:get).with(key).and_return(f)
- expect(f).to receive(:value).and_return(key)
- end
-
- expect(subject).to eq(arguments)
- end
-
- shared_examples 'fallback on cross-slot' do |redirection|
- context 'when redis cluster undergoing slot migration' do
- before do
- allow(pipeline).to receive(:get).and_raise(::Redis::CommandError.new("#{redirection} 1 127.0.0.1:7001"))
- end
-
- it 'logs error and executes sequentially' do
- expect(client).to receive(:_find_node_key).exactly(4).times.and_return(3, 2, 1, 3)
- expect(Gitlab::ErrorTracking).to receive(:log_exception).with(an_instance_of(::Redis::CommandError))
-
- arguments.each do |key|
- expect(redis).to receive(:get).with(key).and_return(key)
- end
-
- subject
- end
- end
- end
-
- it_behaves_like 'fallback on cross-slot', 'MOVED'
- it_behaves_like 'fallback on cross-slot', 'ASK'
-
- context 'when receiving non-MOVED/ASK command errors' do
- before do
- allow(pipeline).to receive(:get).and_raise(::Redis::CommandError.new)
- allow(client).to receive(:_find_node_key).exactly(4).times.and_return(3, 2, 1, 3)
- end
-
- it 'raises error' do
- expect { subject }.to raise_error(::Redis::CommandError)
- end
- end
- end
- end
-end
diff --git a/spec/lib/gitlab/redis/multi_store_spec.rb b/spec/lib/gitlab/redis/multi_store_spec.rb
index 348215d553c03e1b141c75396e0026fc1131fb64..0d841be4e3c642e363b06eea22c4daeeaa37ab9d 100644
--- a/spec/lib/gitlab/redis/multi_store_spec.rb
+++ b/spec/lib/gitlab/redis/multi_store_spec.rb
@@ -58,6 +58,7 @@
context 'when primary_store is not a ::Redis instance' do
before do
allow(primary_store).to receive(:is_a?).with(::Redis).and_return(false)
+ allow(primary_store).to receive(:is_a?).with(::Redis::Cluster).and_return(false)
end
it 'fails with exception' do
@@ -69,6 +70,7 @@
context 'when secondary_store is not a ::Redis instance' do
before do
allow(secondary_store).to receive(:is_a?).with(::Redis).and_return(false)
+ allow(secondary_store).to receive(:is_a?).with(::Redis::Cluster).and_return(false)
end
it 'fails with exception' do
@@ -618,35 +620,6 @@
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 874822e3e6accba692a27f7f6724eff776bf35d3..e822b7399b741c0ac924355d0abe33278f88b936 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.
+ # Check that Gitlab::Redis::Sessions is configured as RedisStore or ClusterStore
it 'instantiates an instance of Redis::Store' do
- expect(store).to be_instance_of(::Redis::Store)
+ expect([::Redis::Store, ::Gitlab::Redis::ClusterStore].include?(store.class)).to eq(true)
end
end
end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 581fd3dfb380ef2aae1adbdc87237466178325e3..9bfb61077a0c8472b3502c7db088c83250f83480 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -539,15 +539,15 @@ def initialize_from_file_path(path)
Rack::Test::UploadedFile.prepend(TouchRackUploadedFile)
-# Monkey-patch to enable ActiveSupport::Notifications for Redis commands
+# Inject middleware to enable ActiveSupport::Notifications for Redis commands
module RedisCommands
module Instrumentation
- def process(commands, &block)
- ActiveSupport::Notifications.instrument('redis.process_commands', commands: commands) do
- super(commands, &block)
+ def call(command, redis_config)
+ ActiveSupport::Notifications.instrument('redis.process_commands', commands: command) do
+ super(command, redis_config)
end
end
end
end
-Redis::Client.prepend(RedisCommands::Instrumentation)
+RedisClient.register(RedisCommands::Instrumentation)
diff --git a/spec/support/helpers/dns_helpers.rb b/spec/support/helpers/dns_helpers.rb
index 0250e4326099ef33df060ef251d89dd3eabab246..27310c7e04c962178df67b91537889d701a2321d 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.map do |redis_instance|
- redis_instance.redis_client_params[:host]
+ hosts = Gitlab::Redis::ALL_CLASSES.flat_map do |redis_instance|
+ redis_instance.params[:host] || redis_instance.params[:nodes]&.map { |n| n[:host] }
end.uniq.compact
hosts.each do |host|
diff --git a/spec/support/matchers/exceed_redis_call_limit.rb b/spec/support/matchers/exceed_redis_call_limit.rb
index 2b1e1ebad23a88b3ae9b972095644025228f2bf8..2cb92089723a90fc0f2b75bc11f307ccb6435c72 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).count
+ @actual = build_recorder(block).by_command(command.to_s).count
@actual > expected
end
diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml
index 22d8871b71877a83cc26c1d27764b4cbb5872210..3a9216e7aca94547ea80ca8018015e791c5feb90 100644
--- a/spec/support/rspec_order_todo.yml
+++ b/spec/support/rspec_order_todo.yml
@@ -6297,7 +6297,6 @@
- './spec/lib/gitlab/instrumentation/rate_limiting_gates_spec.rb'
- './spec/lib/gitlab/instrumentation/redis_base_spec.rb'
- './spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb'
-- './spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb'
- './spec/lib/gitlab/instrumentation/redis_spec.rb'
- './spec/lib/gitlab/internal_post_receive/response_spec.rb'
- './spec/lib/gitlab/issuable/clone/attributes_rewriter_spec.rb'
diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_import/object_import_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_import/object_import_shared_examples.rb
index 3dbe43d822f47556141797d5adf8fc35cb97a0fa..55ce520caab5b7d8e240cf829517748526d8de69 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.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(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.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(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.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(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.perform(project_id, {}, waiter_key) }.to raise_error(StandardError)
+ expect { worker.class.perform_inline(project_id, {}, waiter_key) }.to raise_error(StandardError)
end
end
end
@@ -85,7 +85,7 @@
it 'does not call the importer' do
expect_next(worker.importer_class).not_to receive(:execute)
- worker.perform(project_id, {}, waiter_key)
+ worker.class.perform_inline(project_id, {}, waiter_key)
end
it_behaves_like 'notifies the waiter'
diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb
index 45248f57683b7b5cc59e5f1d08327315b756039d..89eec531b87bcba4f19a3d68126bf18eccaec359 100644
--- a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb
+++ b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/object_import_shared_examples.rb
@@ -19,6 +19,10 @@
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 1c153b7c31b62785f26ac20dae71badef6fb6fc7..2f387528740b6363305791dfa07685b1f5d45046 100644
--- a/spec/support/shared_examples/redis/redis_shared_examples.rb
+++ b/spec/support/shared_examples/redis/redis_shared_examples.rb
@@ -80,18 +80,17 @@
context 'with new format' do
it_behaves_like 'redis store' do
- let(:config_file_name) { config_new_format_host }
+ # use new format host without sentinel details as `.to_s` checks `config` which
+ # tries to resolve master/replica details with an actual sentinel instance.
+ # https://github.com/redis-rb/redis-client/blob/v0.18.0/lib/redis_client/sentinel_config.rb#L128
+ let(:config_file_name) { "spec/fixtures/config/redis_new_format_host_standalone.yml" }
let(:host) { "development-host:#{redis_port}" }
end
end
end
- describe '.redis_client_params' do
- # .redis_client_params wraps over `.redis_store_options` by modifying its outputs
- # to be compatible with `RedisClient`. We test for compatibility in this block while
- # the contents of redis_store_options are tested in the `.params` block.
-
- subject { described_class.new(rails_env).redis_client_params }
+ describe '.params' do
+ subject { described_class.new(rails_env).params }
let(:rails_env) { 'development' }
let(:config_file_name) { config_old_format_socket }
@@ -103,56 +102,6 @@
end
end
- context 'when url is host based' do
- context 'with old format' do
- let(:config_file_name) { config_old_format_host }
-
- it 'does not raise ArgumentError for invalid keywords' do
- expect { RedisClient.config(**subject) }.not_to raise_error
- end
-
- it_behaves_like 'instrumentation_class in custom key'
- end
-
- context 'with new format' do
- let(:config_file_name) { config_new_format_host }
-
- where(:rails_env, :host) do
- [
- %w[development development-host],
- %w[test test-host],
- %w[production production-host]
- ]
- end
-
- with_them do
- it 'does not raise ArgumentError for invalid keywords in SentinelConfig' do
- expect(subject[:name]).to eq(host)
- expect { RedisClient.sentinel(**subject) }.not_to raise_error
- end
-
- it_behaves_like 'instrumentation_class in custom key'
- end
- end
- end
-
- context 'when url contains unix socket reference' do
- let(:config_file_name) { config_old_format_socket }
-
- it 'does not raise ArgumentError for invalid keywords' do
- expect { RedisClient.config(**subject) }.not_to raise_error
- end
-
- it_behaves_like 'instrumentation_class in custom key'
- end
- end
-
- describe '.params' do
- subject { described_class.new(rails_env).params }
-
- let(:rails_env) { 'development' }
- let(:config_file_name) { config_old_format_socket }
-
it 'withstands mutation' do
params1 = described_class.params
params2 = described_class.params
@@ -251,10 +200,16 @@
with_them do
it 'returns hash with host, port, db, and password' do
- is_expected.to include(host: host, password: 'mynewpassword', port: redis_port, db: redis_database)
+ is_expected.to include(name: host, password: 'mynewpassword', db: redis_database)
is_expected.not_to have_key(:url)
end
+
+ it 'does not raise ArgumentError for invalid keywords in SentinelConfig' do
+ expect { RedisClient.sentinel(**subject) }.not_to raise_error
+ end
end
+
+ it_behaves_like 'instrumentation_class in custom key'
end
context 'with redis cluster format' do
@@ -272,13 +227,19 @@
it 'returns hash with cluster and password' do
is_expected.to include(
password: 'myclusterpassword',
- cluster: [
+ nodes: [
{ host: "#{host}1", port: redis_port },
{ host: "#{host}2", port: redis_port }
]
)
is_expected.not_to have_key(:url)
end
+
+ it 'does not raise ArgumentError for invalid keywords in ClusterConfig' do
+ expect { RedisClient::ClusterConfig.new(**subject) }.not_to raise_error
+ end
+
+ it_behaves_like 'instrumentation_class in custom key'
end
end
end
diff --git a/spec/support_specs/helpers/redis_commands/recorder_spec.rb b/spec/support_specs/helpers/redis_commands/recorder_spec.rb
index ef46db5e29ea7071532af7498efffa0b6416853c..5c200f15bae3b7c05f18981cddcb0b18b4021b19 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 d11b044b093bc964a3ba4ff8d6218477203c4e92..4cb0a25f89294a86bea4b7f865234723e7470fb0 100644
--- a/spec/workers/gitlab/github_gists_import/import_gist_worker_spec.rb
+++ b/spec/workers/gitlab/github_gists_import/import_gist_worker_spec.rb
@@ -108,17 +108,24 @@
before do
allow(importer).to receive(:execute).and_return(importer_result)
+ allow(Gitlab::GithubGistsImport::Representation::Gist)
+ .to receive(:from_json_hash)
+ .with(anything)
+ .and_return(gist_object)
end
it 'tracks and logs error' do
+ # use `anything` since jid is created in Sidekiq's middleware. `jid` does not exist until
+ # perform_inline is called.
expect(Gitlab::GithubImport::Logger)
.to receive(:error)
- .with(log_attributes.merge('message' => 'importer failed', 'exception.message' => 'error_message'))
+ .with(log_attributes.merge('message' => 'importer failed', 'exception.message' => 'error_message',
+ 'jid' => anything))
expect(Gitlab::JobWaiter)
.to receive(:notify)
- .with('some_key', subject.jid, ttl: Gitlab::Import::JOB_WAITER_TTL)
+ .with('some_key', anything, ttl: Gitlab::Import::JOB_WAITER_TTL)
- subject.perform(user.id, gist_hash, 'some_key')
+ subject.class.perform_inline(user.id, gist_hash, 'some_key') # perform_inline calls .perform
expect_snowplow_event(
category: 'Gitlab::GithubGistsImport::ImportGistWorker',
@@ -130,7 +137,7 @@
end
it 'persists failure' do
- expect { subject.perform(user.id, gist_hash, 'some_key') }
+ expect { subject.class.perform_inline(user.id, gist_hash, 'some_key') }
.to change { ImportFailure.where(user: user).count }.from(0).to(1)
expect(ImportFailure.where(user_id: user.id).first).to have_attributes(
diff --git a/spec/workers/gitlab/jira_import/import_issue_worker_spec.rb b/spec/workers/gitlab/jira_import/import_issue_worker_spec.rb
index 3b2114ee488132607de9bbc9b6912fc1812a0ac7..c9c4c6e2d2b52a7fc319840301fdd5fc0c21f410 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.perform(project.id, 123, issue_attrs, some_key)
+ subject.class.perform_inline(project.id, 123, issue_attrs, some_key)
expect(label.issues.count).to eq(0)
expect(Gitlab::Cache::Import::Caching.read(Gitlab::JiraImport.failed_issues_counter_cache_key(project.id)).to_i).to eq(0)
@@ -57,7 +57,7 @@
before do
Gitlab::JiraImport.cache_import_label_id(project.id, label.id)
- subject.perform(project.id, 123, issue_attrs, some_key)
+ subject.class.perform_inline(project.id, 123, issue_attrs, some_key)
end
it 'does not record import failure' do
diff --git a/spec/workers/redis_migration_worker_spec.rb b/spec/workers/redis_migration_worker_spec.rb
index 9f29c84a94895f4902f22dd356ac544f64d63db9..d8f2cacdb13e5ee3c7b785020d475b48485f9cae 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
- ::Redis.new(::Gitlab::Redis::Cache.params)
+ ::Gitlab::Redis::Cache.redis
end
end
end