From 43ab15a3d25667a7908ee2ef892dbf4ddff9b445 Mon Sep 17 00:00:00 2001 From: Matt Kemp Date: Wed, 1 Oct 2014 17:16:56 +0000 Subject: Attempt to break the pid lock during startup. This commit attempts to break the pidfilelock during startup in cases where the process may have exited without successfully cleaning up the lockfile. It also attempts to grab the lock before opening the context. Also applied to the Collector module, which may have been looking for the wrong exception since it does not rely on a timeout. --- src/lib/Bcfg2/Reporting/Collector.py | 34 ++++++++++++++++++++++++++-------- src/lib/Bcfg2/Server/BuiltinCore.py | 22 +++++++++++++++------- 2 files changed, 41 insertions(+), 15 deletions(-) (limited to 'src') diff --git a/src/lib/Bcfg2/Reporting/Collector.py b/src/lib/Bcfg2/Reporting/Collector.py index 8ca145f16..50bb49f78 100644 --- a/src/lib/Bcfg2/Reporting/Collector.py +++ b/src/lib/Bcfg2/Reporting/Collector.py @@ -7,7 +7,7 @@ import traceback import threading # pylint: disable=E0611 -from lockfile import LockFailed, LockTimeout +from lockfile import LockFailed, LockTimeout, AlreadyLocked try: from lockfile.pidlockfile import PIDLockFile from lockfile import Error as PIDFileError @@ -118,25 +118,43 @@ class ReportingCollector(object): if self.setup['daemon']: self.logger.debug("Daemonizing") + self.context.pidfile = PIDLockFile(self.setup['daemon']) try: - self.context.pidfile = PIDLockFile(self.setup['daemon']) - self.context.open() + self.context.pidfile.acquire() except LockFailed: self.logger.error("Failed to daemonize: %s" % sys.exc_info()[1]) self.shutdown() return + except AlreadyLocked: + try: # attempt to break the lock + os.kill(self.context.pidfile.read_pid(), 0) + except OSError: # No process with locked PID + self.context.pidfile.break_lock() + else: + self.logger.error("Failed to daemonize: " + "Failed to acquire lock on %s" % + self.setup['daemon']) + self.shutdown() + return except LockTimeout: - self.logger.error("Failed to daemonize: " - "Failed to acquire lock on %s" % - self.setup['daemon']) - self.shutdown() - return + try: # attempt to break the lock + os.kill(self.context.pidfile.read_pid(), 0) + except OSError: # No process with locked PID + self.context.pidfile.break_lock() + else: + self.logger.error("Failed to daemonize: " + "Failed to acquire lock on %s" % + self.setup['daemon']) + self.shutdown() + return except PIDFileError: self.logger.error("Error writing pid file: %s" % traceback.format_exc().splitlines()[-1]) self.shutdown() return + + self.context.open() self.logger.info("Starting daemon") self.transport.start_monitor(self) diff --git a/src/lib/Bcfg2/Server/BuiltinCore.py b/src/lib/Bcfg2/Server/BuiltinCore.py index 93da767c7..aeac161dd 100644 --- a/src/lib/Bcfg2/Server/BuiltinCore.py +++ b/src/lib/Bcfg2/Server/BuiltinCore.py @@ -1,5 +1,6 @@ """ The core of the builtin Bcfg2 server. """ +import os import sys import time import socket @@ -84,18 +85,25 @@ class Core(BaseCore): """ Open :attr:`context` to drop privileges, write the PID file, and daemonize the server core. """ try: - self.context.open() - self.logger.info("%s daemonized" % self.name) - return True + self.context.pidfile.acquire() except LockFailed: err = sys.exc_info()[1] self.logger.error("Failed to daemonize %s: %s" % (self.name, err)) return False except LockTimeout: - err = sys.exc_info()[1] - self.logger.error("Failed to daemonize %s: Failed to acquire lock " - "on %s" % (self.name, self.setup['daemon'])) - return False + try: + os.kill(self.context.pidfile.read_pid(), 0) + except OSError: # No process with locked PID + self.context.pidfile.break_lock() + else: + err = sys.exc_info()[1] + self.logger.error("Failed to daemonize %s: Failed to acquire lock " + "on %s" % (self.name, self.setup['daemon'])) + return False + + self.context.open() + self.logger.info("%s daemonized" % self.name) + return True def _run(self): """ Create :attr:`server` to start the server listening. """ -- cgit v1.2.3-1-g7c22 From 7d11af710bbd1a6fdafcab7ad754953c1f8639b1 Mon Sep 17 00:00:00 2001 From: Matt Kemp Date: Wed, 1 Oct 2014 17:41:52 +0000 Subject: pylint fixes. --- src/lib/Bcfg2/Server/BuiltinCore.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/lib/Bcfg2/Server/BuiltinCore.py b/src/lib/Bcfg2/Server/BuiltinCore.py index aeac161dd..251c39558 100644 --- a/src/lib/Bcfg2/Server/BuiltinCore.py +++ b/src/lib/Bcfg2/Server/BuiltinCore.py @@ -93,12 +93,13 @@ class Core(BaseCore): except LockTimeout: try: os.kill(self.context.pidfile.read_pid(), 0) - except OSError: # No process with locked PID + except OSError: # No process with locked PID self.context.pidfile.break_lock() else: err = sys.exc_info()[1] - self.logger.error("Failed to daemonize %s: Failed to acquire lock " - "on %s" % (self.name, self.setup['daemon'])) + self.logger.error("Failed to daemonize %s: Failed to acquire" + "lock on %s" % (self.name, + self.setup['daemon'])) return False self.context.open() -- cgit v1.2.3-1-g7c22 From 5d7e50dd4f55026390f9a545d0893c3ca51c7a96 Mon Sep 17 00:00:00 2001 From: Matt Kemp Date: Wed, 1 Oct 2014 21:19:20 +0000 Subject: Fixes to ensure pidfile can be opened or broken if stale. --- src/lib/Bcfg2/Reporting/Collector.py | 29 +++++++++++------------------ src/lib/Bcfg2/Server/BuiltinCore.py | 5 ++++- 2 files changed, 15 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/lib/Bcfg2/Reporting/Collector.py b/src/lib/Bcfg2/Reporting/Collector.py index 50bb49f78..336604daa 100644 --- a/src/lib/Bcfg2/Reporting/Collector.py +++ b/src/lib/Bcfg2/Reporting/Collector.py @@ -1,3 +1,4 @@ +import os import sys import atexit import daemon @@ -6,13 +7,12 @@ import time import traceback import threading +from lockfile import LockFailed, LockTimeout # pylint: disable=E0611 -from lockfile import LockFailed, LockTimeout, AlreadyLocked try: - from lockfile.pidlockfile import PIDLockFile - from lockfile import Error as PIDFileError + from daemon.pidfile import TimeoutPIDLockFile except ImportError: - from daemon.pidlockfile import PIDLockFile, PIDFileError + from daemon.pidlockfile import TimeoutPIDLockFile # pylint: enable=E0611 import Bcfg2.Logger @@ -118,7 +118,9 @@ class ReportingCollector(object): if self.setup['daemon']: self.logger.debug("Daemonizing") - self.context.pidfile = PIDLockFile(self.setup['daemon']) + self.context.pidfile = TimeoutPIDLockFile(self.setup['daemon'], + acquire_timeout=5) + # Attempt to ensure lockfile is able to be created and not stale try: self.context.pidfile.acquire() except LockFailed: @@ -126,17 +128,6 @@ class ReportingCollector(object): sys.exc_info()[1]) self.shutdown() return - except AlreadyLocked: - try: # attempt to break the lock - os.kill(self.context.pidfile.read_pid(), 0) - except OSError: # No process with locked PID - self.context.pidfile.break_lock() - else: - self.logger.error("Failed to daemonize: " - "Failed to acquire lock on %s" % - self.setup['daemon']) - self.shutdown() - return except LockTimeout: try: # attempt to break the lock os.kill(self.context.pidfile.read_pid(), 0) @@ -144,8 +135,8 @@ class ReportingCollector(object): self.context.pidfile.break_lock() else: self.logger.error("Failed to daemonize: " - "Failed to acquire lock on %s" % - self.setup['daemon']) + "Failed to acquire lock on %s" % + self.setup['daemon']) self.shutdown() return except PIDFileError: @@ -153,6 +144,8 @@ class ReportingCollector(object): traceback.format_exc().splitlines()[-1]) self.shutdown() return + else: + self.context.pidfile.release() self.context.open() self.logger.info("Starting daemon") diff --git a/src/lib/Bcfg2/Server/BuiltinCore.py b/src/lib/Bcfg2/Server/BuiltinCore.py index 251c39558..ed959f78d 100644 --- a/src/lib/Bcfg2/Server/BuiltinCore.py +++ b/src/lib/Bcfg2/Server/BuiltinCore.py @@ -84,6 +84,7 @@ class Core(BaseCore): def _daemonize(self): """ Open :attr:`context` to drop privileges, write the PID file, and daemonize the server core. """ + # Attempt to ensure lockfile is able to be created and not stale try: self.context.pidfile.acquire() except LockFailed: @@ -91,7 +92,7 @@ class Core(BaseCore): self.logger.error("Failed to daemonize %s: %s" % (self.name, err)) return False except LockTimeout: - try: + try: # attempt to break the lock os.kill(self.context.pidfile.read_pid(), 0) except OSError: # No process with locked PID self.context.pidfile.break_lock() @@ -101,6 +102,8 @@ class Core(BaseCore): "lock on %s" % (self.name, self.setup['daemon'])) return False + else: + self.context.pidfile.release() self.context.open() self.logger.info("%s daemonized" % self.name) -- cgit v1.2.3-1-g7c22 From 5038a6278a1700671141ec58ea4a3cc5ce799e3b Mon Sep 17 00:00:00 2001 From: Matt Kemp Date: Thu, 2 Oct 2014 18:15:59 +0000 Subject: Catch possible typeerror resulting from None being returned when reading the pid. --- src/lib/Bcfg2/Reporting/Collector.py | 2 +- src/lib/Bcfg2/Server/BuiltinCore.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/lib/Bcfg2/Reporting/Collector.py b/src/lib/Bcfg2/Reporting/Collector.py index 336604daa..2ff09d483 100644 --- a/src/lib/Bcfg2/Reporting/Collector.py +++ b/src/lib/Bcfg2/Reporting/Collector.py @@ -131,7 +131,7 @@ class ReportingCollector(object): except LockTimeout: try: # attempt to break the lock os.kill(self.context.pidfile.read_pid(), 0) - except OSError: # No process with locked PID + except (OSError, TypeError): # No process with locked PID self.context.pidfile.break_lock() else: self.logger.error("Failed to daemonize: " diff --git a/src/lib/Bcfg2/Server/BuiltinCore.py b/src/lib/Bcfg2/Server/BuiltinCore.py index ed959f78d..29beb35d5 100644 --- a/src/lib/Bcfg2/Server/BuiltinCore.py +++ b/src/lib/Bcfg2/Server/BuiltinCore.py @@ -94,7 +94,7 @@ class Core(BaseCore): except LockTimeout: try: # attempt to break the lock os.kill(self.context.pidfile.read_pid(), 0) - except OSError: # No process with locked PID + except (OSError, TypeError): # No process with locked PID self.context.pidfile.break_lock() else: err = sys.exc_info()[1] -- cgit v1.2.3-1-g7c22 From d1b630dc6edb77f248c2c5bcaddf7526210a55da Mon Sep 17 00:00:00 2001 From: Matt Kemp Date: Thu, 2 Oct 2014 18:16:24 +0000 Subject: Remove PIDFileError as it does not always exist in the package and is rarely used. --- src/lib/Bcfg2/Reporting/Collector.py | 5 ----- 1 file changed, 5 deletions(-) (limited to 'src') diff --git a/src/lib/Bcfg2/Reporting/Collector.py b/src/lib/Bcfg2/Reporting/Collector.py index 2ff09d483..8e2fe1cb1 100644 --- a/src/lib/Bcfg2/Reporting/Collector.py +++ b/src/lib/Bcfg2/Reporting/Collector.py @@ -139,11 +139,6 @@ class ReportingCollector(object): self.setup['daemon']) self.shutdown() return - except PIDFileError: - self.logger.error("Error writing pid file: %s" % - traceback.format_exc().splitlines()[-1]) - self.shutdown() - return else: self.context.pidfile.release() -- cgit v1.2.3-1-g7c22