Add logic to daemonizer to be able to kill what it daemonizes

Add logic to daemonizer to be able to kill what it daemonizes

Previously, daemonzer.py simply launched a daemon process without
storing its pid anywhere. Consequently, it was left to the daemon
itself to handle shutting down and/or restarting. This cl adds the
option to store the daemon's pid in a file and kill it.

This update is part of the effort to add temperature monitoring to
android devices. A more advanced daemon manager would certainly help.

BUG=519884
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296613

[bpastene, 2015-09-04 16:48:41]

bpastene@chromium.org changed reviewers:
+ luqui@chromium.org

[bpastene, 2015-09-05 00:25:46]

ptal when you get a chance

[luqui, 2015-09-08 18:18:10]

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py
File scripts/slave/daemonizer.py (right):

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:30: def start(cmd, pid_file_path):
seems like this action ought to be called 'restart'

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:33: if (pid_file_path == None):
You don't need parens around if's conditional in python.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:42: if (pid):
no parens

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:49: except (OSError):
no parens

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:100: with open(pid_file_path, 'w+') as pid_file:
I think you mean 'w' mode instead of 'w+'.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:105: proc.communicate()
This will make daemonize block, which is the opposite of what is good

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:119: logging.exception('Error terminating daemon
process')
Log the exception here so we have some idea what the error was.

[bpastene, 2015-09-08 20:50:47]

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py
File scripts/slave/daemonizer.py (right):

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:30: def start(cmd, pid_file_path):
On 2015/09/08 18:18:10, luqui wrote:
> seems like this action ought to be called 'restart'

Done.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:33: if (pid_file_path == None):
On 2015/09/08 18:18:10, luqui wrote:
> You don't need parens around if's conditional in python.

Done.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:42: if (pid):
On 2015/09/08 18:18:10, luqui wrote:
> no parens

Done.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:49: except (OSError):
On 2015/09/08 18:18:10, luqui wrote:
> no parens

Done.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:100: with open(pid_file_path, 'w+') as pid_file:
On 2015/09/08 18:18:10, luqui wrote:
> I think you mean 'w' mode instead of 'w+'.

Done.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:105: proc.communicate()
On 2015/09/08 18:18:10, luqui wrote:
> This will make daemonize block, which is the opposite of what is good

Previously, it would do just that, and return the child's return code once it
terminated. But, as far as I can tell, that information isn't used anywhere (not
that it even could be since this is a fork of a dead fork). I'll go ahead and
change it.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:119: logging.exception('Error terminating daemon
process')
On 2015/09/08 18:18:10, luqui wrote:
> Log the exception here so we have some idea what the error was.

logging.exception() automatically logs the exception details and stack trace of
the caught exception

[luqui, 2015-09-08 21:15:57]

lgtm after undoing the thing I told you to do.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py
File scripts/slave/daemonizer.py (right):

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:105: proc.communicate()
On 2015/09/08 20:50:47, bpastene wrote:
> On 2015/09/08 18:18:10, luqui wrote:
> > This will make daemonize block, which is the opposite of what is good
> 
> Previously, it would do just that, and return the child's return code once it
> terminated. But, as far as I can tell, that information isn't used anywhere
(not
> that it even could be since this is a fork of a dead fork). I'll go ahead and
> change it.

Oh I misunderstood, I missed the fork() above.  I think commuicate() is correct
after all.

https://codereview.chromium.org/1328623004/diff/1/scripts/slave/daemonizer.py...
scripts/slave/daemonizer.py:119: logging.exception('Error terminating daemon
process')
On 2015/09/08 20:50:47, bpastene wrote:
> On 2015/09/08 18:18:10, luqui wrote:
> > Log the exception here so we have some idea what the error was.
> 
> logging.exception() automatically logs the exception details and stack trace
of
> the caught exception

Oh cool :-)

[bpastene, 2015-09-09 19:40:48]

The CQ bit was checked by bpastene@chromium.org

[bpastene, 2015-09-09 19:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from luqui@chromium.org
Link to the patchset: https://codereview.chromium.org/1328623004/#ps40001
(title: "Parent of daemon waits again")

[commit-bot: I haz the power, 2015-09-09 19:40:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1328623004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1328623004/40001

[commit-bot: I haz the power, 2015-09-09 19:43:39]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296613