From 4db6e6a48082134f1ddaef49ee58b32b5221c780 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Wed, 22 Apr 2020 16:17:44 +0200 Subject: [PATCH 1/3] Gemfiles: use fork of redis-rb --- Gemfile.base | 6 +++++- Gemfile.lock | 10 ++++++++-- Gemfile.on_prem.lock | 10 ++++++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/Gemfile.base b/Gemfile.base index 6afc56f1d..9becac63c 100644 --- a/Gemfile.base +++ b/Gemfile.base @@ -52,7 +52,6 @@ gem 'daemons', '= 1.2.4' # Production gems gem 'rake', '~> 13.0' gem 'builder', '= 3.2.3' -gem 'redis', '= 4.1.1' # Use a patched resque to allow reusing their Airbrake Failure class gem 'resque', git: 'https://github.com/3scale/resque', branch: '3scale' gem 'rack', '~> 2.0.8' @@ -63,3 +62,8 @@ gem 'bugsnag', '~> 6', require: nil gem 'yabeda-prometheus', '~> 0.5.0' gem 'async-redis', '~> 0.4.1' gem 'falcon', '~> 0.35' + +# Use a patched redis-rb that fixes an issue when trying to connect with +# sentinels and avoids retrying calls when there's a timeout to prevent +# duplicated commands. It's based on version 4.1.3. +gem 'redis', git: 'https://github.com/3scale/redis-rb', branch: 'apisonator' diff --git a/Gemfile.lock b/Gemfile.lock index 4051a5733..2c2215fe1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -5,6 +5,13 @@ GIT specs: puma (2.15.3) +GIT + remote: https://github.com/3scale/redis-rb + revision: 35301b3d952975300a4cb30d408ae3b5969d0440 + branch: apisonator + specs: + redis (4.1.3) + GIT remote: https://github.com/3scale/resque revision: 88839e71756ea9b6edfc9426a0af71e94109c135 @@ -182,7 +189,6 @@ GEM rack-test (0.8.2) rack (>= 1.0, < 3) rake (13.0.1) - redis (4.1.1) redis-namespace (1.6.0) redis (>= 3.0.4) reentrant_flock (0.1.1) @@ -297,7 +303,7 @@ DEPENDENCIES rack (~> 2.0.8) rack-test (~> 0.8.2) rake (~> 13.0) - redis (= 4.1.1) + redis! resque! resque_spec (~> 0.17.0) resque_unit (~> 0.4.4)! diff --git a/Gemfile.on_prem.lock b/Gemfile.on_prem.lock index 0a9808e7d..0f8aa8824 100644 --- a/Gemfile.on_prem.lock +++ b/Gemfile.on_prem.lock @@ -5,6 +5,13 @@ GIT specs: puma (2.15.3) +GIT + remote: https://github.com/3scale/redis-rb + revision: 35301b3d952975300a4cb30d408ae3b5969d0440 + branch: apisonator + specs: + redis (4.1.3) + GIT remote: https://github.com/3scale/resque revision: 88839e71756ea9b6edfc9426a0af71e94109c135 @@ -170,7 +177,6 @@ GEM rack-test (0.8.2) rack (>= 1.0, < 3) rake (13.0.1) - redis (4.1.1) redis-namespace (1.6.0) redis (>= 3.0.4) reentrant_flock (0.1.1) @@ -278,7 +284,7 @@ DEPENDENCIES rack (~> 2.0.8) rack-test (~> 0.8.2) rake (~> 13.0) - redis (= 4.1.1) + redis! resque! resque_spec (~> 0.17.0) resque_unit (~> 0.4.4)! From 084df6665cc00450b66c0f46441142c82abd3ab8 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Wed, 22 Apr 2020 16:18:31 +0200 Subject: [PATCH 2/3] extensions: delete redis-rb patch It's included in the 3scale fork. --- lib/3scale/backend.rb | 1 - lib/3scale/backend/extensions/redis.rb | 44 -------------------------- 2 files changed, 45 deletions(-) delete mode 100644 lib/3scale/backend/extensions/redis.rb diff --git a/lib/3scale/backend.rb b/lib/3scale/backend.rb index c69817465..62ae5a60c 100644 --- a/lib/3scale/backend.rb +++ b/lib/3scale/backend.rb @@ -5,7 +5,6 @@ require 'hiredis' require 'redis' -require '3scale/backend/extensions/redis' require 'resque' require 'securerandom' diff --git a/lib/3scale/backend/extensions/redis.rb b/lib/3scale/backend/extensions/redis.rb deleted file mode 100644 index d56c45bbe..000000000 --- a/lib/3scale/backend/extensions/redis.rb +++ /dev/null @@ -1,44 +0,0 @@ -# Monkey-patches a method in Redis::Client::Connector::Sentinel to fix a bug -# with sentinel passwords. It applies the fix in -# https://github.com/redis/redis-rb/pull/856. -# -# The fix was included in 4.1.2, but we cannot upgrade because that version -# drops support for ruby < 2.3.0 which we still need to support. -# -# This should only be temporary. It should be deleted when updating the gem. -class Redis - class Client - class Connector - class Sentinel - def sentinel_detect - @sentinels.each do |sentinel| - client = Redis::Client.new(@options.merge({:host => sentinel[:host], - :port => sentinel[:port], - password: sentinel[:password], - :reconnect_attempts => 0, - })) - - begin - if result = yield(client) - # This sentinel responded. Make sure we ask it first next time. - @sentinels.delete(sentinel) - @sentinels.unshift(sentinel) - - return result - end - rescue BaseConnectionError - rescue RuntimeError => exception - # Needed because when the sentinel address cannot be resolved it - # raises this instead of "BaseConnectionError" - raise unless exception.message =~ /Name or service not known/ - ensure - client.disconnect - end - end - - raise CannotConnectError, "No sentinels available." - end - end - end - end -end From 805c5e9f77d9d4ca49199c39f0c27f7ac38b49a8 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Wed, 22 Apr 2020 16:23:02 +0200 Subject: [PATCH 3/3] storage_helpers: set reconnect_attempts to 1 --- lib/3scale/backend/storage_helpers.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/3scale/backend/storage_helpers.rb b/lib/3scale/backend/storage_helpers.rb index d67d2e5a1..bcd3db20d 100644 --- a/lib/3scale/backend/storage_helpers.rb +++ b/lib/3scale/backend/storage_helpers.rb @@ -44,9 +44,13 @@ class << self connect_timeout: 5, read_timeout: 3, write_timeout: 3, - # this is set to zero to avoid potential double transactions - # see https://github.com/redis/redis-rb/issues/668 - reconnect_attempts: 0, + # Note that we can set reconnect_attempts to >= 0 because we use + # our redis-rb fork which implements a workaround for this issue + # that shows that when there might be duplicated transactions when + # there's a timeout: https://github.com/redis/redis-rb/issues/668 + # We should investigate if there are edge cases that can lead to + # duplicated commands because of this setting. + reconnect_attempts: 1, # use by default the C extension client driver: :hiredis }.freeze