Add health checks for rabbitmq #1345

jaym · 2017-07-31T14:13:45Z

This PR adds health checks for rabbitmq for modules that make use of rabbitmq.

stevendanna · 2017-07-31T14:19:38Z

src/oc_erchef/apps/chef_index/src/chef_index.erl

+        rabbitmq ->
+            chef_index_queue:ping(envy:get(chef_index, rabbitmq_vhost, binary));
+        _ ->
+            ping


Should this be pong?

stevendanna · 2017-07-31T14:22:57Z

omnibus/files/private-chef-cookbooks/private-chef/recipes/rabbitmq.rb

@@ -218,6 +218,13 @@
    retries 10
  end

+  execute "#{rmq_ctl} set_permissions -p #{rabbitmq['vhost']} #{rabbitmq['management_user']} \".*\" \".*\" \".*\"" do


These calls are expensive, any way we could combine this with the one below?

I checked the docs, it does not look like i can pass a regex for vhost

stevendanna · 2017-07-31T15:03:26Z

omnibus/files/private-chef-cookbooks/private-chef/templates/default/oc_erchef.config.erb

@@ -227,6 +231,24 @@
            {max_age, <%= @solr_http_max_age %>},
            {max_connection_duration, <%= @solr_http_max_connection_duration %>},
            {ibrowse_options, <%= @solr_ibrowse_options %>}
+        ]},
+        {rabbitmq_index_management_service, [


It is a bit of a shame that in the common case (actions & index rabbitmq being the same) that we are going to have 2 pools of HTTP connections. However, since thy can be different, I think this is cleanest for now.

jaym · 2017-08-01T13:48:24Z

src/oc_erchef/apps/chef_index/src/chef_index_sup.erl

@@ -28,7 +28,8 @@
 %% API
 -export([
         server_for_vhost/1,
-         start_link/0
+         start_link/0,
+         maybe_rabbitmq_monitoring/0


remove from exports

stevendanna · 2017-08-01T14:34:13Z

src/oc_erchef/apps/oc_chef_wm/test/chef_wm_actions_queue_monitoring_tests.erl

+     [
+      {"it's alive",
+       fun() ->
+            meck:new(oc_httpc),


You can put this in the setup fun (the first fun after forearch) if you call it in each case.

stevendanna

Overall this looks good. Thanks for working on it! I've left a few minor comments. I agree with your TODO comment, but I'd also be fine merging this and doing the refactors in another PR to keep everything reviewable.

It might be nice if you squashed some of those commits and had a least 1 commit that describes the new feature in a few sentences.

We probably also want to put this in the RELEASE_NOTES?

stevendanna · 2017-08-02T13:28:27Z

src/oc_erchef/apps/chef_index/src/chef_index_queue.erl

+
+-spec ping(binary()) -> pong | pang.
+ping(VHost) ->
+    % TODO: chef_wm_rabbitmq_management needs to be moved to a shared app


[nit] The TODO is fine but we are trying to use TODO(ssd) 2017-08-02: (i.e. TODO(initials) DATE:) so we know how long they've been there and who added them without crawling through git history.

updated, added a little more detail as well

stevendanna · 2017-08-02T13:34:06Z

src/oc_erchef/apps/oc_chef_wm/src/oc_chef_action_queue_config.erl

+        ]).
+
+-define(SERVER, ?MODULE).
+-define(POOLNAME, rabbitmq_management_service).


[minor] Perhaps rabbitmq_actions_management_service to match rabbitmq_index_management_service

stevendanna · 2017-08-02T13:36:17Z

src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_sup.erl

@@ -91,10 +92,11 @@ maybe_start_action(false, Workers) ->
    Workers.


-check_queue_at_capacity(Vhost, Queue) ->
+check_actions_queue_at_capacity(Vhost, Queue) ->


[minor] Perhaps pass the pool name since we've already looked it up at the only call site of this function.

stevendanna · 2017-08-02T13:39:42Z

src/oc_erchef/apps/oc_chef_wm/src/chef_wm_rabbitmq_management.erl

@@ -302,3 +241,14 @@ parse_integer(Val) when is_list(Val) ->
        {Int, _Rest} -> Int
    end;
 parse_integer(_) -> undefined.
+
+-spec check_aliveness(atom(), string()) -> true | false.


[nit] true | false can be boolean(). You can find the various built-in typespecs here: http://erlang.org/doc/reference_manual/typespec.html

stevendanna · 2017-08-02T13:44:44Z

src/oc_erchef/apps/oc_chef_wm/src/chef_wm_actions_queue_monitoring.erl

-                                                                            Queue,
-                                                                            Dropped),
+                      chef_wm_rabbitmq_management:check_current_queue_state(
+                        oc_chef_action_queue_config:get_rabbit_management_pool_name(),


[minor] Perhaps we can throw the poolname in the queue_monitor_state in init() so we don't have to look it up every time. This lookup is cheap so it isn't the end of the world to look it up, but since we already have the Vhost and Queue there it would be nice to be consistent (and some microseconds faster I suppose).

jaym · 2017-08-02T14:56:20Z

Let me do some rebasing to tell the story i want

stevendanna

Nice work Jay!

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

… vips The current chef server configuration allows people to point actions at a rabbitmq that is not the same as the chef_index one. This change will allow us to check the health of each one. Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Saves us on having to do a look up every call Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

…vice We have a management service for the internal queue as well now, so this will specifically describe it without additional context needed Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

This feature adds checks for RabbiMQ vhosts. These are checked when the `_status` endpoint is called, and will use the RabbitMQ management plugin to check the health of the vhosts we use. Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

stevendanna reviewed Jul 31, 2017

View reviewed changes

jaym force-pushed the jdm/SUSTAIN-672-improved-healthcheck branch from d6342cd to d5a9fc4 Compare July 31, 2017 14:50

stevendanna reviewed Jul 31, 2017

View reviewed changes

jaym force-pushed the jdm/SUSTAIN-672-improved-healthcheck branch 2 times, most recently from 7c703f5 to ba36e6c Compare August 1, 2017 00:26

jaym commented Aug 1, 2017

View reviewed changes

stevendanna reviewed Aug 1, 2017

View reviewed changes

jaym force-pushed the jdm/SUSTAIN-672-improved-healthcheck branch 3 times, most recently from 09f17b7 to db17116 Compare August 1, 2017 20:18

stevendanna reviewed Aug 2, 2017

View reviewed changes

jaym force-pushed the jdm/SUSTAIN-672-improved-healthcheck branch 4 times, most recently from 84694ef to e647298 Compare August 2, 2017 16:55

stevendanna approved these changes Aug 3, 2017

View reviewed changes

Jay Mundrawala and others added 10 commits August 4, 2017 08:12

Add healthcheck for analytics rabbitmq vhost

e38e259

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Make dev env work without /etc/timezone

a2cdaeb

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Add ping for chef_index that checks rabbitmq

d1b8796

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Add aliveness tests

eb01196

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Add oc_chef_action:ping() tests

2c280f0

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Add ping test for chef_index

a686770

Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Save pool name as part of process state

e4efcdf

Saves us on having to do a look up every call Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

Rename rabbitmq_management_service to rabbitmq_actions_management_ser…

e287601

…vice We have a management service for the internal queue as well now, so this will specifically describe it without additional context needed Signed-off-by: Jay Mundrawala <jmundrawala@chef.io>

jaym force-pushed the jdm/SUSTAIN-672-improved-healthcheck branch from 1de5992 to 96b687d Compare August 4, 2017 15:13

jaym requested a review from a team August 4, 2017 15:13

stevendanna approved these changes Aug 4, 2017

View reviewed changes

jaym merged commit b8b147e into master Aug 7, 2017

jaym deleted the jdm/SUSTAIN-672-improved-healthcheck branch August 7, 2017 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add health checks for rabbitmq #1345

Add health checks for rabbitmq #1345

jaym commented Jul 31, 2017 •

edited

Loading

stevendanna Jul 31, 2017

jaym Jul 31, 2017

stevendanna Jul 31, 2017

jaym Jul 31, 2017

stevendanna Jul 31, 2017

stevendanna Jul 31, 2017

jaym Aug 1, 2017

stevendanna Aug 1, 2017

stevendanna left a comment

stevendanna Aug 2, 2017 •

edited

Loading

jaym Aug 2, 2017

stevendanna Aug 2, 2017

jaym Aug 2, 2017

stevendanna Aug 2, 2017

jaym Aug 2, 2017

stevendanna Aug 2, 2017

jaym Aug 2, 2017

stevendanna Aug 2, 2017

jaym Aug 2, 2017

jaym commented Aug 2, 2017

stevendanna left a comment

Add health checks for rabbitmq #1345

Add health checks for rabbitmq #1345

Conversation

jaym commented Jul 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

stevendanna Aug 2, 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

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

jaym commented Aug 2, 2017

stevendanna left a comment

Choose a reason for hiding this comment

jaym commented Jul 31, 2017 •

edited

Loading

stevendanna Aug 2, 2017 •

edited

Loading