Merge lp:~notmyname/swift/daemon_lock into lp:~hudson-openstack/swift/trunk

[John Dickinson]

Added optional flock to daemon to prevent processes overrunning themselves.

Added this option to swift-log-stats-collector

[gholt (gholt)]

I'm a bit confused on this. Isn't this what swift-init tries to accomplish already with its pid files?

[gholt (gholt)]

This has been sitting here a while, so I figured I'd update folks that don't sit in our office:

My general complaint on this is that previously the only things that used run_daemon were actually daemons and launched via swift-init, which does pid file style locking including stale pid file checking, 'reload' support, etc.

This change does something quite similar, but not quite, for a thing that isn't a daemon anyway, with no record of the previous pid (for sending it signals or whatever).

review: Disapprove

Unmerged revisions

148. By John Dickinson on 2010-12-14

made locking in daemon better (including tests)

147. By John Dickinson on 2010-12-13

added lock_file argument to swift-log-stats-collector

146. By John Dickinson on 2010-12-13

added locking (flock) to daemon.py

=== modified file 'bin/swift-log-stats-collector'
--- bin/swift-log-stats-collector	2010-11-11 22:41:07 +0000
+++ bin/swift-log-stats-collector	2010-12-14 03:55:09 +0000
@@ -15,12 +15,14 @@
 # limitations under the License.
 
 from swift.stats.log_processor import LogProcessorDaemon
-from swift.common.utils import parse_options
+from swift.common.utils import parse_options, readconf
 from swift.common.daemon import run_daemon
 
 if __name__ == '__main__':
     conf_file, options = parse_options()
     # currently the LogProcessorDaemon only supports run_once
     options['once'] = True
+    conf = readconf(conf_file, 'log-processor')
+    lock_file = conf.get('lock_file', '/var/run/swift/stats.lock')
     run_daemon(LogProcessorDaemon, conf_file, section_name=None,
-               log_name='log-stats-collector', **options)
+               log_name='log-stats-collector', lock_file=lock_file, **options)

=== modified file 'etc/log-processing.conf-sample'
--- etc/log-processing.conf-sample	2010-10-04 21:12:43 +0000
+++ etc/log-processing.conf-sample	2010-12-14 03:55:09 +0000
@@ -9,6 +9,7 @@
 # lookback_hours = 120
 # lookback_window = 120
 # user = swift
+# lock_file = /var/run/swift/stats.lock
 
 [log-processor-access]
 # log_dir = /var/log/swift/

=== modified file 'swift/common/daemon.py'
--- swift/common/daemon.py	2010-11-19 18:15:41 +0000
+++ swift/common/daemon.py	2010-12-14 03:55:09 +0000
@@ -17,9 +17,29 @@
 import sys
 import signal
 from re import sub
+import contextlib
+import fcntl
 from swift.common import utils
 
 
[e-mail]
+def flock(path, logger):
+    if path:
+        fd = os.open(path, os.O_CREAT | os.O_RDWR)
+        try:
+            fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+        except IOError:
+            logger.error('Already running... cooling the jets now.')
+            sys.exit(2)
+        try:
+            yield fd
+        finally:
+            os.close(fd)
+            os.unlink(path)
+    else:
+        yield None
+
+
 class Daemon(object):
     """Daemon base class"""
 
@@ -35,7 +55,7 @@
         """Override this to run forever"""
         raise NotImplementedError('run_forever not implemented')
 
-    def run(self, once=False, **kwargs):
+    def run(self, once=False, lock_file=None, **kwargs):
         """Run the daemon"""
         utils.validate_configuration()
         utils.capture_stdio(self.logger, **kwargs)
@@ -48,14 +68,16 @@
 
         signal.signal(signal.SIGTERM, kill_children)
 
-        if once:
-            self.run_once()
-        else:
-            self.run_forever()
+        with flock(lock_file, self.logger):
+            # if lock_file evaluates to false, nothing will be locked
+            if once:
+                self.run_once()
+            else:
+                self.run_forever()
 
 
 def run_daemon(klass, conf_file, section_name='',
-               once=False, **kwargs):
+               once=False, lock_file=None, **kwargs):
     """
     Loads settings from conf, then instantiates daemon "klass" and runs the
     daemon with the specified once kwarg.  The section_name will be derived
@@ -66,6 +88,7 @@
     :param conf_file: Path to configuration file
     :param section_name: Section name from conf file to load config from
     :param once: Passed to daemon run method
+    :param lock_file: path to file to use as a lock file
     """
     # very often the config section_name is based on the class name
     # the None singleton will be passed through to readconf as is
@@ -85,7 +108,7 @@
         logger = utils.get_logger(conf, conf.get('log_name', section_name),
                                   log_to_console=kwargs.pop('verbose', False))
     try:
-        klass(conf).run(once=once, **kwargs)
+        klass(conf).run(once=once, lock_file=lock_file, **kwargs)
     except KeyboardInterrupt:
         logger.info('User quit')
     logger.info('Exited')

=== modified file 'test/unit/common/test_daemon.py'
--- test/unit/common/test_daemon.py	2010-11-19 18:15:41 +0000
+++ test/unit/common/test_daemon.py	2010-12-14 03:55:09 +0000
@@ -89,6 +89,22 @@
             daemon.run_daemon(MyDaemon, conf_file, once=True)
             self.assertEquals(MyDaemon.once_called, True)
 
+            # test lock file (to make sure that code path doesn't break)
+            old_flock = daemon.fcntl.flock
+
+            def new_flock_no_error(*a):
+                pass
+
+            def new_flock_error(*a):
+                raise IOError
+            daemon.fcntl.flock = new_flock_no_error
+            daemon.run_daemon(MyDaemon, conf_file,
+                              lock_file='/tmp/foo_lock_test')
+            daemon.fcntl.flock = new_flock_error
+            self.assertRaises(SystemExit, daemon.run_daemon, MyDaemon,
+                              conf_file, lock_file='/tmp/foo_lock_test')
+            daemon.fcntl.flock = old_flock
+
             # test raise in daemon code
             MyDaemon.run_once = MyDaemon.run_raise
             self.assertRaises(OSError, daemon.run_daemon, MyDaemon,