APERTA-9087 Configure rails+sidekiq for redis sentinel in tahi/ihat #2965

tadp · 2017-03-27T22:49:26Z

JIRA issue: https://developer.plos.org/jira/browse/APERTA-9087

What this PR does:

This implements Redis Sentinel for sidekiq in tahi and ihat.

Dependent Prs:

IHAT pr: https://github.com/Tahi-project/ihat/pull/134

aperta-formula pr: https://github.com/PLOS-Formulas/aperta-formula/pull/31

Notes:

QA: This deploys correctly when the right settings are in place. I don't think there is much else to check. @egh and I also verified the failover of the sentinel setup.

Code Review Tasks:

Reviewer tasks:

I skimmed the code; it makes sense
I read the code; it looks good
I ran the code (in the review environment or locally)
I performed a 5 minute walkthrough of the site looking for oddities
I agree the code fulfills the Acceptance Criteria
I agree the author has fulfilled their tasks

After the Code Review:

Author tasks:

The Product Team has reviewed and approved this feature

egh · 2017-03-27T22:56:44Z

config/initializers/sidekiq.rb

@@ -1,7 +1,7 @@
 sentinels = ENV['REDIS_SENTINELS']
 Sidekiq.configure_server do |config|
  if sentinels.present?
-    sentinel_list = sentinels.map do |sentinel_host|
+    sentinel_list = sentinels.join(',').map do |sentinel_host|


I think you mean split(/,/)

egh · 2017-03-27T23:00:16Z

config/initializers/sidekiq.rb

@@ -1,7 +1,7 @@
 sentinels = ENV['REDIS_SENTINELS']
 Sidekiq.configure_server do |config|
  if sentinels.present?
-    sentinel_list = sentinels.map do |sentinel_host|
+    sentinel_list = sentinels.join(',').map do |sentinel_host|
      { host: sentinel_host, port: ENV['REDIS_PORT'] }
    end.join(',')


Per https://brigade.engineering/high-availability-sidekiq-with-redis-sentinel-2c562e65ecd2 the key sentinels expects an array. Why are we joining using ,?

egh · 2017-03-27T23:02:15Z

Can you explain what master_name is? Does this need to be configurable?

egh · 2017-03-27T23:27:09Z

config/initializers/sidekiq.rb

@@ -31,9 +34,9 @@

 Sidekiq.configure_client do |config|
  if sentinels.present?
-    sentinel_list = sentinels.map do |sentinel_host|
+    sentinel_list = sentinels.split(',').map do |sentinel_host|


We should not copy & paste code.

egh · 2017-03-27T23:28:43Z

config/initializers/sidekiq.rb

+# YAML is part of the Ruby standard library and it is used here to convert
+# "['server1', 'server2', 'server3']" into ['server1', 'server2', 'server3']
+require 'yaml'
+sentinels = YAML::load(ENV['REDIS_SENTINELS'])


When you are parsing a string dumped by some other process, we need to know that what we are parsing is what the other process is dumping. What is salt dumping? Just because YAML 'works' doesn't mean this is an appropriate solution

Salt is dumping YAML and defaults to YAML for its settings, so we can safely assume that this will work. https://docs.saltstack.com/en/latest/topics/tutorials/starting_states.html

@tadp, I don't think this is true. YAML is the default format for .sls files, but the means by which salt serializes python objects in a jinja template is another matter I think, it might just use whatever default serializing python uses (print).

When setting up a system like this, where one application is serializing information and another is deserializing, I think it's important to have a well-established contract that's easy to use. To this end, I think we should keep the deserializing logic in TahiEnv. It would be great to add a validator for array values that ensures that they're serialized properly in our env files. It would be pretty cool to just write

TahiEnv.redis_sentinels

and automatically get a ruby list back instead of some string that still needs further parsing.

Nice. Yeah, we should be extending TahiEnv here.

Great idea everyone. I'll move the YAML:load into TahiEnv and include a test for TahiEnv to make sure it is converting the string correctly.

egh · 2017-03-27T23:30:25Z

config/initializers/sidekiq.rb

 Sidekiq.configure_server do |config|
  if sentinels.present?
    sentinel_list = sentinels.map do |sentinel_host|
      { host: sentinel_host, port: ENV['REDIS_PORT'] }
-    end.join(',')
+    end


I still don't understand this. The sidekiq configuration seems to want an array of values URLs. Why are we passing it an array of hashes? See https://brigade.engineering/high-availability-sidekiq-with-redis-sentinel-2c562e65ecd2

So when I tried URLs initially, everything would break with an error because it was expecting HOST and PORT to be defined. This is one of the things I would like to test in a sentinel environment.

egh · 2017-03-27T23:32:34Z

It should be possible to test this code in your local vagrant by modifying your aperta/vagrant.sls file to include the correct configuration, then running vagrant provision, then deploying to vagrant using cap vagrant deploy, then trying to start the app in your vagrant. If we can't test this locally, I have a feeling it's going to take a long time to get this right if we have to merge to master every time we make a change.

tadp · 2017-03-28T00:10:40Z

@egh I'll revert the other related merges in a separate pr and try in vagrant tomorrow before proceeding with this pr

surfacedamage · 2017-03-29T20:43:28Z

Hey @tadp, I didn't explore this PR in detail and you may have already done so, but could you verify that switching to sentinel still works with the health check endpoint and especially on the endpoint on the heroku review apps?

tadp · 2017-03-29T20:55:28Z

@surfacedamage The heroku review apps and our development environment should be unaffected because it will use the previous settings unless REDIS_SENTINEL_ENABLED is 'true'

I plan to do some testing in the CI and Stage environments soon and I'll look out for health check issues. Thanks for the heads up.

…v var

egh · 2017-04-05T23:40:52Z

config/initializers/sidekiq.rb

+redis_config = if TahiEnv.redis_sentinel_enabled?
+                 {
+                   master_name: 'aperta',
+                   sentinels: TahiEnv.redis_sentinels,


This config doesn't actually work. This does:

redis_config = if TahiEnv.redis_sentinel_enabled? { url: ENV[ENV['REDIS_PROVIDER'] || 'REDIS_URL'], role: :master, master_name: 'aperta', sentinels: TahiEnv.redis_sentinels.map do |s| u = URI.parse(s) { host: u.host, port: (u.port || 26_379) } end, failover_reconnect_timeout: 20, namespace: "tahi_#{Rails.env}" } else { namespace: "tahi_#{Rails.env}" } end

See https://github.com/redis/redis-rb#sentinel-support

More consistent than rolling our own.

…dis-sentinel

bnbeckwith · 2017-04-25T18:14:02Z

@egh and I walked through the change to ensure that this deploys with the right configuration. We also took down one of the redis instances to see it failover (while watching the health_check route). Everything worked as expected.

bnbeckwith · 2017-04-25T20:13:18Z

This can go in when these are complete:

IHat: https://github.com/Tahi-project/ihat/pull/134
Plos formulas: https://github.com/PLOS-Formulas/aperta-formula/pull/31

…dis-sentinel

- 25 workers is already the default

…dis-sentinel

phoca2004 temporarily deployed to ciagent-stage-pr-2965 March 27, 2017 22:49 Inactive

tadp added the needs review label Mar 27, 2017

egh reviewed Mar 27, 2017

View reviewed changes

tadp had a problem deploying to ciagent-stage-pr-2965 March 27, 2017 23:19 Failure

tadp assigned egh Mar 27, 2017

tadp added in review and removed needs review labels Mar 27, 2017

egh reviewed Mar 27, 2017

View reviewed changes

tadp force-pushed the APERTA-9087-configure-rails-sidekiq-for-redis-sentinel branch from c8d495f to 3259e0c Compare March 27, 2017 23:31

tadp had a problem deploying to ciagent-stage-pr-2965 March 27, 2017 23:31 Failure

tadp added the do not merge label Mar 28, 2017

tadp force-pushed the APERTA-9087-configure-rails-sidekiq-for-redis-sentinel branch from 3259e0c to 3acbe14 Compare March 28, 2017 21:05

tadp had a problem deploying to ciagent-stage-pr-2965 March 28, 2017 21:05 Failure

phoca2004 temporarily deployed to ciagent-stage-pr-2965 March 28, 2017 21:55 Inactive

egh added wip and removed in review labels Mar 28, 2017

tadp force-pushed the APERTA-9087-configure-rails-sidekiq-for-redis-sentinel branch from 3acbe14 to 2355d3b Compare March 28, 2017 23:55

phoca2004 temporarily deployed to ciagent-stage-pr-2965 March 28, 2017 23:55 Inactive

tadp force-pushed the APERTA-9087-configure-rails-sidekiq-for-redis-sentinel branch from 2355d3b to f2a1338 Compare March 29, 2017 17:44

tadp temporarily deployed to ciagent-stage-pr-2965 March 29, 2017 17:44 Inactive

egh changed the title ~~APERTA-9087 Split the string to make it into an array~~ APERTA-9087 Enable redis sentinel for sidekiq Mar 29, 2017

tadp changed the title ~~APERTA-9087 Enable redis sentinel for sidekiq~~ APERTA-9087 Configure rails+sidekiq for redis sentinel in tahi/ihat Mar 29, 2017

tadp force-pushed the APERTA-9087-configure-rails-sidekiq-for-redis-sentinel branch from f2a1338 to 8690559 Compare March 29, 2017 20:56

tadp added 2 commits April 2, 2017 15:19

APERTA-9087 Refactor configs and remove unused SIDEKIQ_CONCURRENCY en…

6c15212

…v var

APERTA-9087 Add specs for the env array validator

3031e43

tadp force-pushed the APERTA-9087-configure-rails-sidekiq-for-redis-sentinel branch from 225319d to 3031e43 Compare April 2, 2017 22:19

tadp temporarily deployed to ciagent-stage-pr-2965 April 2, 2017 22:19 Inactive

egh reviewed Apr 5, 2017

View reviewed changes

APERTA-9087 Use sidekiq redis connection setup for health check

147096d

More consistent than rolling our own.

phoca2004 deployed to ciagent-stage-pr-2965 April 6, 2017 15:54 View deployment

egh added on hold wip and removed in review needs reviewer response labels Apr 6, 2017

egh and others added 5 commits April 21, 2017 12:56

Merge branch 'master' into APERTA-9087-configure-rails-sidekiq-for-re…

e0435ee

…dis-sentinel

APERTA-9087 Fix redis sentinel configuration

18569c1

APERTA-9087 Update redis gem to 3.3.3

01fc070

APERTA-9087 Set :host option to redis sentinel

db55ee1

Merge branch 'master' into APERTA-9087-configure-rails-sidekiq-for-re…

9c1235e

…dis-sentinel

bnbeckwith added needs PO acceptance and removed on hold wip labels Apr 25, 2017

bnbeckwith and others added 5 commits April 25, 2017 16:17

Merge branch 'master' into APERTA-9087-configure-rails-sidekiq-for-re…

f483267

…dis-sentinel

APERTA-9087 Remove incorrect comment and do not set default

daa9e0f

- 25 workers is already the default

APERTA-9087 Simplify redis setup

53b1d21

Merge branch 'master' into APERTA-9087-configure-rails-sidekiq-for-re…

28091ee

…dis-sentinel

Merge branch 'master' into APERTA-9087-configure-rails-sidekiq-for-re…

eb360fe

…dis-sentinel

bnbeckwith added merge when green and removed needs PO acceptance labels Apr 27, 2017

bnbeckwith merged commit 6ce6a16 into master Apr 27, 2017

bnbeckwith deleted the APERTA-9087-configure-rails-sidekiq-for-redis-sentinel branch April 27, 2017 02:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APERTA-9087 Configure rails+sidekiq for redis sentinel in tahi/ihat #2965

APERTA-9087 Configure rails+sidekiq for redis sentinel in tahi/ihat #2965

tadp commented Mar 27, 2017 •

edited by bnbeckwith

Loading

egh Mar 27, 2017

egh Mar 27, 2017

egh commented Mar 27, 2017

egh Mar 27, 2017

egh Mar 27, 2017

tadp Mar 28, 2017 •

edited

Loading

JackLaBarba Mar 28, 2017

egh Mar 28, 2017

tadp Mar 28, 2017

egh Mar 27, 2017

tadp Mar 27, 2017

egh commented Mar 27, 2017

tadp commented Mar 28, 2017 •

edited

Loading

surfacedamage commented Mar 29, 2017

tadp commented Mar 29, 2017

egh Apr 5, 2017 •

edited

Loading

bnbeckwith commented Apr 25, 2017

bnbeckwith commented Apr 25, 2017 •

edited

Loading

APERTA-9087 Configure rails+sidekiq for redis sentinel in tahi/ihat #2965

APERTA-9087 Configure rails+sidekiq for redis sentinel in tahi/ihat #2965

Conversation

tadp commented Mar 27, 2017 • edited by bnbeckwith Loading

What this PR does:

Dependent Prs:

IHAT pr: https://github.com/Tahi-project/ihat/pull/134

aperta-formula pr: https://github.com/PLOS-Formulas/aperta-formula/pull/31

Notes:

Code Review Tasks:

After the Code Review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egh commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadp Mar 28, 2017 • edited Loading

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

egh commented Mar 27, 2017

tadp commented Mar 28, 2017 • edited Loading

surfacedamage commented Mar 29, 2017

tadp commented Mar 29, 2017

egh Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

bnbeckwith commented Apr 25, 2017

bnbeckwith commented Apr 25, 2017 • edited Loading

tadp commented Mar 27, 2017 •

edited by bnbeckwith

Loading

tadp Mar 28, 2017 •

edited

Loading

tadp commented Mar 28, 2017 •

edited

Loading

egh Apr 5, 2017 •

edited

Loading

bnbeckwith commented Apr 25, 2017 •

edited

Loading