Issue 3211 - Integrate customservice::supervisor with notification daemon

Issue 3211 - Integrate customservice::supervisor with notification daemon

[Wladimir Palant, 2015-10-21 11:05:27]

https://codereview.adblockplus.org/29329307/diff/29329308/hiera/roles/filters...
File hiera/roles/filterserver.yaml (right):

https://codereview.adblockplus.org/29329307/diff/29329308/hiera/roles/filters...
hiera/roles/filterserver.yaml:10: spawn-fcgi:
/var/run/500-multiplexer_spawn-fcgi.pid
The script expects a relative path, 500-multiplexer_spawn-fcgi.pid will work.
While specifying /var/run explicitly works as well, I don't think that we should
do this.

Also, this is rather unfortunate because the pid file name here depends both on
internals of the notificationserver and spawn-fcgi modules. While it doesn't
seem that we can avoid the latter (no way to specify complete PID file name,
only individual parts of it), we definitely can address the former. This
directive should be part of the notificationserver module rather than being set
in Hiera. Also, notificationserver should explicitly specify order => 500 on the
pool rather than implying the default.

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
File modules/sitescripts/manifests/keep_alive_services.pp (right):

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
modules/sitescripts/manifests/keep_alive_services.pp:5: class
sitescripts::keep_alive_services {
I don't think that this belongs in the sitescripts module - we use that module
to ensure that sitescripts are set up, these scripts aren't being used here.
There should be a new keepalive module or something like that.

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
File modules/sitescripts/templates/keep_alive_services.erb (right):

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
modules/sitescripts/templates/keep_alive_services.erb:2: # Via Puppet; see
https://issues.adblockplus.org/ticket/3211
This comment makes little sense unless we only plan to use this functionality
for filter list servers (and I'm pretty sure that we don't).

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
modules/sitescripts/templates/keep_alive_services.erb:6: <%=   name %> =
/var/run/<%= name %>.pid
Please specify a relative path here as well.

[mathias, 2015-10-21 11:44:01]

https://codereview.adblockplus.org/29329307/diff/29329308/hiera/roles/filters...
File hiera/roles/filterserver.yaml (right):

https://codereview.adblockplus.org/29329307/diff/29329308/hiera/roles/filters...
hiera/roles/filterserver.yaml:10: spawn-fcgi:
/var/run/500-multiplexer_spawn-fcgi.pid
On 2015/10/21 11:05:26, Wladimir Palant wrote:
> The script expects a relative path, 500-multiplexer_spawn-fcgi.pid will work.
> While specifying /var/run explicitly works as well, I don't think that we
should
> do this.

Why don't you think so? Given the other circumstances, especially that we cannot
really auto-aggregate that path or configure it explicitly, I actually prefer
being explicit in the configuration file at least.

> Also, this is rather unfortunate because the pid file name here depends both
on
> internals of the notificationserver and spawn-fcgi modules. While it doesn't
> seem that we can avoid the latter (no way to specify complete PID file name,
> only individual parts of it), we definitely can address the former. This
> directive should be part of the notificationserver module rather than being
set
> in Hiera. Also, notificationserver should explicitly specify order => 500 on
the
> pool rather than implying the default.

Indeed, an explicit $order should be set, will update the patch-set accordingly.

I also believe that a more sophisticated approach would not require hard-coding
in Hiera in the current fashion, but some sort of service supervisor module to
incorporate where necessary.

However, I do not see any way to implement that based on
sitescripts.management.bin.start_services in a resonable fashion without prior
modification to sitescripts.bin.utils.get_config or sitescripts::configfragment
or both. Unfortunately any reasonable approach I came up with so far is way more
overhead than just coding a new solution from scratch, and either way it will
require more coordination upfront and more excessive testing before roll-out.

I do not consider this effort reasonable right now, especially given the
severity of the intention behind this patch-set. But if you have an idea I may
have overseen, please share it with me.

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
File modules/sitescripts/manifests/keep_alive_services.pp (right):

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
modules/sitescripts/manifests/keep_alive_services.pp:5: class
sitescripts::keep_alive_services {
On 2015/10/21 11:05:26, Wladimir Palant wrote:
> I don't think that this belongs in the sitescripts module - we use that module
> to ensure that sitescripts are set up, these scripts aren't being used here.
> There should be a new keepalive module or something like that.

I believe that a separation is not necessary: It is quite normal that a Puppet
module handles both the installation of a software or package, while sub-modules
handle the setup of features contained within. From infrastructure perspective,
I do not see any reason why this should not apply to sitescripts just as it
applies to 3rd party software.

However, that consideration of course heavily depends on the currently proposed
approach itself.

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
File modules/sitescripts/templates/keep_alive_services.erb (right):

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
modules/sitescripts/templates/keep_alive_services.erb:2: # Via Puppet; see
https://issues.adblockplus.org/ticket/3211
On 2015/10/21 11:05:26, Wladimir Palant wrote:
> This comment makes little sense unless we only plan to use this functionality
> for filter list servers (and I'm pretty sure that we don't).

Acknowledged.

[Wladimir Palant, 2015-10-21 12:07:07]

https://codereview.adblockplus.org/29329307/diff/29329308/hiera/roles/filters...
File hiera/roles/filterserver.yaml (right):

https://codereview.adblockplus.org/29329307/diff/29329308/hiera/roles/filters...
hiera/roles/filterserver.yaml:10: spawn-fcgi:
/var/run/500-multiplexer_spawn-fcgi.pid
On 2015/10/21 11:44:01, mathias wrote:
> Why don't you think so? Given the other circumstances, especially that we
cannot
> really auto-aggregate that path or configure it explicitly, I actually prefer
> being explicit in the configuration file at least.

I don't think I can follow but - well, I won't insist on relative paths, it's
not worth discussing.

> Indeed, an explicit $order should be set, will update the patch-set
accordingly.

I hope that you will also move the keep_alive_services invocation into
notificationserver module rather than leaving it in Hiera - that was the more
important part.

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
File modules/sitescripts/manifests/keep_alive_services.pp (right):

https://codereview.adblockplus.org/29329307/diff/29329308/modules/sitescripts...
modules/sitescripts/manifests/keep_alive_services.pp:5: class
sitescripts::keep_alive_services {
On 2015/10/21 11:44:01, mathias wrote:
> I believe that a separation is not necessary: It is quite normal that a Puppet
> module handles both the installation of a software or package, while
sub-modules
> handle the setup of features contained within.

That's not how we use sitescripts, and it really isn't a package - it's rather a
collection of fairly unrelated scripts used for all kinds of things.
Concentrating all invocations in the sitescripts module will only make things
unnecessarily complicated.

[mathias, 2015-10-21 12:08:19]

0fde7ff Issue 3211 - Remove senseless comment
74e792f Issue 3211 - Explicitly ensure pool order 500

[mathias, 2015-10-21 12:13:37]

On 2015/10/21 12:07:07, Wladimir Palant wrote:
> I hope that you will also move the keep_alive_services invocation into
> notificationserver module rather than leaving it in Hiera - that was the more
> important part.

Ok, I see, will try to find a suitable way to do that.
 
> That's not how we use sitescripts, and it really isn't a package - it's rather
a
> collection of fairly unrelated scripts used for all kinds of things.
> Concentrating all invocations in the sitescripts module will only make things
> unnecessarily complicated.

Very well, I agree on your view of sitescripts, actually thought I would be
quite alone with that opinion anyway.

In addition, I believe that not leaking implementation details (like using
sitescripts to provide a service supervisor) is a good practice in Puppet as
well. But it may become a bit tricky to provide a sufficent interface to the
service watching/restarting feature based on the techniques available, but let's
see what I can come up with.

[Wladimir Palant, 2015-10-27 13:21:54]

LGTM