Issue 8963083: sitescripts: Script to keep services alive (Closed)
|
Created:
Nov. 29, 2012, 2:47 p.m. by Felix Dahlke Modified:
Nov. 30, 2012, 2:33 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionThis script starts services that should be running, but aren't. The pid file names have to be configured manually in /etc/sitescripts.
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #MessagesTotal messages: 6
http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... File sitescripts/management/bin/start_services.py (right): http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:41: return True That still needs to be written, right? http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:47: pid_file = "/var/run/%s" % services[service] Use os.path.join() here? http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:49: if not _files_exist([init_file, pid_file]): Actually, if the pid file doesn't exist it means that the service isn't running - we should start it. And if the init file doesn't exist we should fail - otherwise we might end up mistakenly relying on this script even though we have a typo in our configuration. http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:52: pid_file_content = open(pid_file).read().rstrip() I generally prefer to close files explicitly... http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:62: print "%s is not running, starting ..." % service You are aware that any output from a cron job results in a mail to administrator, right? Then again, maybe that's not a bad thing - if services need restarting then this justifies a notification.
Uploaded a new patch set, all issues addressed. http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... File sitescripts/management/bin/start_services.py (right): http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:41: return True On 2012/11/29 15:01:46, Wladimir Palant wrote: > That still needs to be written, right? Um, yes. That's sort of important :) Done. http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:47: pid_file = "/var/run/%s" % services[service] On 2012/11/29 15:01:46, Wladimir Palant wrote: > Use os.path.join() here? Done. http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:49: if not _files_exist([init_file, pid_file]): On 2012/11/29 15:01:46, Wladimir Palant wrote: > Actually, if the pid file doesn't exist it means that the service isn't running > - we should start it. And if the init file doesn't exist we should fail - > otherwise we might end up mistakenly relying on this script even though we have > a typo in our configuration. Done. http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:52: pid_file_content = open(pid_file).read().rstrip() On 2012/11/29 15:01:46, Wladimir Palant wrote: > I generally prefer to close files explicitly... Done. http://codereview.adblockplus.org/8963083/diff/1/sitescripts/management/bin/s... sitescripts/management/bin/start_services.py:62: print "%s is not running, starting ..." % service On 2012/11/29 15:01:46, Wladimir Palant wrote: > You are aware that any output from a cron job results in a mail to > administrator, right? Then again, maybe that's not a bad thing - if services > need restarting then this justifies a notification. Yes, that's what I intended. A crashing service is something we'll want to look into.
LGTM but you might want to address some of the comments before pushing. http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... File sitescripts/management/bin/start_services.py (right): http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... sitescripts/management/bin/start_services.py:26: keys = set(section_keys) - set(default_keys) Strange indentation here, one space instead of two? http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... sitescripts/management/bin/start_services.py:34: return os.path.isdir(os.path.join("/proc", str(pid))) I might have preferred os.kill(..., 0) but this will do as well. http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... sitescripts/management/bin/start_services.py:41: with open(pid_path) as file: pid_string = file.read() Does this really have to be all on one line?
Issues addressed, uploaded a new patch set. http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... File sitescripts/management/bin/start_services.py (right): http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... sitescripts/management/bin/start_services.py:26: keys = set(section_keys) - set(default_keys) On 2012/11/30 10:46:07, Wladimir Palant wrote: > Strange indentation here, one space instead of two? Done. http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... sitescripts/management/bin/start_services.py:34: return os.path.isdir(os.path.join("/proc", str(pid))) On 2012/11/30 10:46:07, Wladimir Palant wrote: > I might have preferred os.kill(..., 0) but this will do as well. Didn't know that trick, using it now. http://codereview.adblockplus.org/8963083/diff/5001/sitescripts/management/bi... sitescripts/management/bin/start_services.py:41: with open(pid_path) as file: pid_string = file.read() On 2012/11/30 10:46:07, Wladimir Palant wrote: > Does this really have to be all on one line? Done.
Still LGTM :) 
|
