Sentinel support #79

brodock · 2016-10-20T05:14:24Z

Sentinel with the ruby client is a little tricky to get right, because they use a special convention in "Redis URL" to pass additional params.

When you have sentinel, it always handle the first connection your client will make to, and it redirects your to a working "master".

For a production setup, you need at least 3 independent machines, each one running redis and sentinel. One of these machines will have a master redis, the others will have slave redis.

For sake of testing, you can do 1 machine with 1 sentinel and 1 master and 1 slave redis running in different ports.

Here is a minimal sentinel.conf file:

bind 0.0.0.0
port 26379
sentinel monitor myredis 127.0.0.1 6379 1
sentinel down-after-milliseconds myredis 10000
sentinel auth-pass myredis "mypassword"

myredis is the <master-name> as stated in sentinel.conf documentation: http://download.redis.io/redis-stable/sentinel.conf

With that setup, here is how you connect to sentinel with Redis client (in ruby):

client = ::Redis.new(url: 'redis://:mypassword@myredis/', sentinels: [host: '127.0.0.1', port: 26379]
client.info # to make sure it worked, check output.

What can go wrong?
This is the most common error: Redis::CannotConnectError: No sentinels available. and the most frustating one to debug.

It can mean literally anything. Your password is wrong, you are pointing to an unreachable address, invalid port, you typed the wrong "master-name" in the url, etc.

With this PR you can make MailRoom use Sentinel to connect to Redis for both Sidekiq and Arbitration

Here is a sample config.yml:

---
:mailboxes:
  -
    :email: "user1@domain.tld"
    :password: "mypassword"
    :name: "inbox"
    :search_command: 'NEW'
    :host: 127.0.0.1
    :port: 1430
    :ssl: false
    :delivery_method: sidekiq
    :delivery_options:
      :redis_url: "redis://:mypassword@myredis:6379"
      :sentinels:
        -
          :host: 127.0.0.1
          :port: 26379
    :arbitration_method: redis
    :arbitration_options:
      :redis_url: "redis://:mypassword@myredis:6379"
      :sentinels:
        -
          :host: 127.0.0.1
          :port: 26379

fixes #55

maxlazio · 2016-10-20T17:49:56Z

@DouweM Could you check this out?

tpitale · 2016-10-20T18:06:45Z

.gitlab-ci.yml

@@ -0,0 +1,31 @@
+# Cache gems in between builds


MailRoom uses travis ci: https://travis-ci.org/tpitale/mail_room.

Are there reasons why we would also run gitlab ci?

Hi @tpitale, this was there because my initial fork was running here: https://gitlab.com/brodock/mail_room/tree/sentinel-support so I needed that file to run it with our CI.

It should do no harm, but I can remove it from the commit if you want. (as a side-effect, anyone who clone / fork / mirror using GitLab gets CI enabled automatically if the file stays)

tpitale · 2016-10-20T18:08:16Z

lib/mail_room/arbitration/redis.rb

        def initialize(mailbox)
          redis_url = mailbox.arbitration_options[:redis_url] || "redis://localhost:6379"
          namespace = mailbox.arbitration_options[:namespace]
+          sentinels = mailbox.arbitration_options[:sentinels]


What is the structure of sentinels? Would you mind updating the README to include usage for this configuration?

tpitale · 2016-10-20T18:09:15Z

lib/mail_room/arbitration/redis.rb

-          redis = ::Redis.new(url: options.redis_url)
+          sentinels = options.sentinels
+          if sentinels
+            redis = ::Redis.new(url: options.redis_url, sentinels: sentinels)


What happens if sentinels is just an empty array? Does Redis ignore that option? Is it possible to simply default the value to an empty array if no option is set (and thus avoid the conditional and local variable)?

unfortunately no: https://github.com/redis/redis-rb/blob/d75708f6e05d9bba7dfec65880f9a55252f3fd2a/lib/redis/client.rb#L89

Blargh. Okay, thanks.

tpitale · 2016-10-20T18:11:00Z

spec/lib/arbitration/redis_spec.rb

+
+    context 'when sentinel is present' do
+      let(:redis_url) { 'redis://:mypassword@sentinel-master:6379' }
+      let(:sentinels) { [host: '10.0.0.1', port: '26379'] }


Is this configuration intended to be an array of hashes? Could we make that more explicit (adding {} or an additional sentinel config would make it more clear)?

@brodock Doesn't look done :)

o.0 ohhh i fixed the other spec that is similar, will commit to this one.

tpitale · 2016-10-20T18:12:13Z

spec/lib/arbitration/redis_spec.rb

+        )
+      }
+
+      before { ::Redis::Client::Connector::Sentinel.any_instance.stubs(:resolve).returns(sentinels) }


This any_instance stubbing worries me, just a little. Combined with the instance_variable_get in the test has me curious.

It's really hard to make any kind of test/mock for sentinel... I've opted here for checking library API contract, but I'm open to any idea you can have. (this is very far from a full integration test)

I've opened an issue to have RedisMock available in the main Gem or into a separated Gem: redis/redis-rb#641

This would allow us to make a better test.

tpitale · 2016-10-20T18:12:34Z

Thanks for your work @brodock!

tpitale · 2016-10-21T14:30:59Z

@DouweM anything you'd like to add?

DouweM · 2016-10-21T15:04:07Z

lib/mail_room/arbitration/redis.rb

+          if sentinels
+            redis = ::Redis.new(url: options.redis_url, sentinels: sentinels)
+          else
+            redis = ::Redis.new(url: options.redis_url)


I don't really like repeating ::Redis.new(url: options.redis_url). What do you think about:

redis_options = {} redis_options[:url] = options.redis_url redis_options[:sentinels] = options.sentinels if options.sentinels ::Redis.new(redis_options)

Or (one line shorter 😸 ):

redis_options = {url: options.redis_url} redis_options[:sentinels] = options.sentinels if options.sentinels ::Redis.new(redis_options)

DouweM · 2016-10-21T15:04:28Z

lib/mail_room/delivery/sidekiq.rb

-        client = Redis.new(url: options.redis_url)
+        sentinels = options.sentinels
+        if sentinels
+          client = Redis.new(url: options.redis_url, sentinels: sentinels)


Same comment as https://github.com/tpitale/mail_room/pull/79/files#r84497595!

tpitale · 2016-10-21T17:32:17Z

Travis failed due to GH outage. Will retest in a bit.

tpitale · 2016-10-21T17:39:25Z

Okay, now it's failing for some specific reason:

 1) MailRoom::Delivery::Sidekiq#options when only redis_url is specified client has same specified redis_url
     Failure/Error: return nil, nil unless ui
     SystemStackError:
       stack level too deep
     # /home/travis/.rvm/rubies/ruby-2.0.0-p598/lib/ruby/2.0.0/uri/generic.rb:557
     # 
     #   Showing full backtrace because every line was filtered out.
     #   See docs for RSpec::Configuration#backtrace_exclusion_patterns and
     #   RSpec::Configuration#backtrace_inclusion_patterns for more information.

brodock · 2016-10-24T12:31:26Z

@tpitale it's fixed.

tpitale · 2016-10-24T19:32:46Z

@brodock ready for me to merge?

brodock · 2016-10-24T22:27:12Z

@tpitale yes :)

tpitale · 2016-10-25T03:16:50Z

Released v0.9.0. Thanks again @brodock.

brodock and others added 4 commits August 18, 2016 03:30

Add .gitlab-ci.yml to build on GitLab CI

fde5d69

WIP: Sentinel support

1be36d6

Added specs for redis and sidekiq connection credentials

cf35b55

Improved redis connector specs for Sentinel

654c1d8

brodock mentioned this pull request Oct 20, 2016

Redis sentinel support #55

Closed

tpitale reviewed Oct 20, 2016

View reviewed changes

Fixed spec and added Sentinel documentation

d2436de

DouweM reviewed Oct 21, 2016

View reviewed changes

brodock added 2 commits October 21, 2016 18:06

Improved documentation for Sentinel

905c79f

Codestyle

409ebac

Fix stacklevel too deep

bee8c80

tpitale merged commit 4e6016c into tpitale:master Oct 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sentinel support #79

Sentinel support #79

brodock commented Oct 20, 2016 •

edited

maxlazio commented Oct 20, 2016

tpitale Oct 20, 2016

brodock Oct 20, 2016 •

edited

tpitale Oct 20, 2016

brodock Oct 21, 2016

tpitale Oct 20, 2016

brodock Oct 21, 2016

tpitale Oct 21, 2016

tpitale Oct 20, 2016

brodock Oct 21, 2016

DouweM Oct 21, 2016

brodock Oct 21, 2016

tpitale Oct 20, 2016

brodock Oct 20, 2016 •

edited

brodock Oct 20, 2016 •

edited

tpitale commented Oct 20, 2016

tpitale commented Oct 21, 2016

DouweM Oct 21, 2016

tpitale Oct 21, 2016

DouweM Oct 21, 2016

tpitale commented Oct 21, 2016

tpitale commented Oct 21, 2016

brodock commented Oct 24, 2016

tpitale commented Oct 24, 2016

brodock commented Oct 24, 2016

tpitale commented Oct 25, 2016

Sentinel support #79

Sentinel support #79

Conversation

brodock commented Oct 20, 2016 • edited

maxlazio commented Oct 20, 2016

Choose a reason for hiding this comment

brodock Oct 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brodock Oct 20, 2016 • edited

Choose a reason for hiding this comment

brodock Oct 20, 2016 • edited

Choose a reason for hiding this comment

tpitale commented Oct 20, 2016

tpitale commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpitale commented Oct 21, 2016

tpitale commented Oct 21, 2016

brodock commented Oct 24, 2016

tpitale commented Oct 24, 2016

brodock commented Oct 24, 2016

tpitale commented Oct 25, 2016

brodock commented Oct 20, 2016 •

edited

brodock Oct 20, 2016 •

edited

brodock Oct 20, 2016 •

edited

brodock Oct 20, 2016 •

edited