From 8190ed4059b7be6c8c0edde146afd1ba3157f14b Mon Sep 17 00:00:00 2001 From: Tyler Ramer Date: Thu, 18 Jun 2020 16:13:22 -0400 Subject: [PATCH] Remove lockfile from mainUtils [Lockfile](https://pypi.org/project/lockfile/) has not been maintained since around 2015. Further, the functionality it provided seems poor - a review of the code indicated that it used the presence of the PID file itself as the lock - in Unix, using a file's existence followed by a creation is not atomic, so a lock could be prone to race conditions. The lockfile package also did not clean up after itself - a process which was destroyed unexpectedly would not clear the created locks, so some faulty logic was added to mainUtils.py, which checked to see if a process with the same PID as the lockfile's creator was running. This is obviously failure prone, as a new process might be assigned the same PID as the old lockfile's owner, without actually being the same process. (Of note, the SIG_DFL argument to os.kill() is not a signal at all, but rather of type signal.handler. It appears that the python cast this handler to the int 0, which, according to man 2 kill, leads to no signal being sent, but existance and permission checks are still performed. So it is a happy accident that this code worked at all) This commit removes lockfile from the codebase entirely. It also adds a "PIDLockFile" class which provides an atomic-guarenteed lock via the mkdir and rmdir commands on Unix - thus, it is not safely portable to Windows, but this should not be an issue as only Unix-based utilities use the "simple_main()" function. PIDLockFile provides API compatible classes to replace most of the functionality from lockfile.PidLockFile, but does remove any timeout logic as it was not used in any meaningful sense - a hard-coded timeout of 1 second was used, but an immediate result of if the lock is held is sufficient. PIDLockFile also includes appropriate __enter__, __exit__, and __del__ attributes, so that, should we extend this class in the future, with syntax is functional, and __del__ calls release, so a process reaped unexpectedly should still clean its own locks as part of the garbage collection process. Authored-by: Tyler Ramer --- NOTICE | 22 ---- README.amazon_linux | 2 +- README.docker.md | 2 +- README.ubuntu.bash | 1 - concourse/scripts/combine_cli_coverage.bash | 1 - gpMgmt/Makefile | 3 - gpMgmt/bin/Makefile | 22 +--- gpMgmt/bin/gpmovemirrors | 7 +- gpMgmt/bin/gppylib/mainUtils.py | 110 +++++++++++++++--- .../bin/gppylib/programs/clsRecoverSegment.py | 4 +- .../gppylib/test/unit/test_unit_mainUtils.py | 80 +++++++++++++ python-dependencies.txt | 1 - src/tools/docker/centos6/Dockerfile | 2 +- src/tools/docker/centos7/Dockerfile | 2 +- src/tools/docker/ubuntu/Dockerfile | 1 - 15 files changed, 183 insertions(+), 77 deletions(-) create mode 100755 gpMgmt/bin/gppylib/test/unit/test_unit_mainUtils.py diff --git a/NOTICE b/NOTICE index edf60ec3c1..845c9b13e4 100644 --- a/NOTICE +++ b/NOTICE @@ -1701,28 +1701,6 @@ OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. -================================================================= ---------------------- pylockfile (MIT) ---------------------------- -================================================================= - -COPYRIGHT AND PERMISSION NOTICE - -Copyright (c) 2007 Skip Montanaro. - -All rights reserved. - -Permission to use, copy, modify, and distribute this software for any purpose -with or without fee is hereby granted, provided that the above copyright -notice and this permission notice appear in all copies. - -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS. IN -NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, -DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR -OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE -OR OTHER DEALINGS IN THE SOFTWARE. - ================================================================= --------------------- Proj (MIT) ---------------------------- ================================================================= diff --git a/README.amazon_linux b/README.amazon_linux index 91099cf916..6e52512c52 100644 --- a/README.amazon_linux +++ b/README.amazon_linux @@ -38,7 +38,7 @@ cat /sys/kernel/mm/transparent_hugepage/enabled # pip packages curl https://bootstrap.pypa.io/get-pip.py | sudo python -sudo pip install --upgrade setuptools wheel pip lockfile psutil +sudo pip install --upgrade setuptools wheel pip psutil # configure and build git clone https://github.com/greenplum-db/gpdb.git diff --git a/README.docker.md b/README.docker.md index e7b23c40f3..9070813b27 100644 --- a/README.docker.md +++ b/README.docker.md @@ -35,7 +35,7 @@ gpdb_src/concourse/scripts/setup_gpadmin_user.bash echo "/usr/sbin/sshd" >> /root/.bashrc su - gpadmin -pip install --user psutil lockfile +pip install --user psutil cat >> ~/.bash_profile < $(LOCKFILE_DST)/__init__.py - awk 'BEGIN{print "# pylint: disable-all"} { print }' < `echo $(LOCKFILE_SRC)/pidlockfile.py` > $(LOCKFILE_DST)/pidlockfile.py - subprocess32: @echo "--- subprocess32, Linux only" @if [ `uname -s` = 'Linux' ]; then \ diff --git a/gpMgmt/bin/gpmovemirrors b/gpMgmt/bin/gpmovemirrors index 83a87b5e50..2a59cd5c2a 100755 --- a/gpMgmt/bin/gpmovemirrors +++ b/gpMgmt/bin/gpmovemirrors @@ -37,7 +37,7 @@ GPDB_STOPPED = 1 GPDB_STARTED = 2 GPDB_UTILITY = 3 -GP_MOVEMIRROS_PID_FILE = "gpmovemirrors.pid" +GP_MOVEMIRRORS_LOCK_PATH = "gpmovemirrors.lock" description = (""" Moves mirror segments in a pre-existing GPDB Array. @@ -311,11 +311,12 @@ try: enable_verbose_logging() - mainOptions = {"pidfilename": GP_MOVEMIRROS_PID_FILE} + mainOptions = {"pidlockpath": GP_MOVEMIRRORS_LOCK_PATH} sml = SimpleMainLock(mainOptions) otherpid = sml.acquire() if otherpid is not None: - logger.error("An instance of %s is already running (pid %s)" % ("gpmovemirrors", otherpid)) + logger.error("Lockfile %s indicates that an instance of %s is already running with PID %s" % (sml.ppath, "gpmovemirrors", otherpid)) + logger.error("If this is not the case, remove the lockfile directory at %s" % (sml.ppath)) sys.exit(1) logger.info("Invocation of gpmovemirrors mirrors") diff --git a/gpMgmt/bin/gppylib/mainUtils.py b/gpMgmt/bin/gppylib/mainUtils.py index 75b60b0f05..d248b2f0c4 100644 --- a/gpMgmt/bin/gppylib/mainUtils.py +++ b/gpMgmt/bin/gppylib/mainUtils.py @@ -16,7 +16,7 @@ extend common functions of our gp utilities. Please keep this in mind and try to avoid placing logic for a specific utility here. """ -import os, sys, signal, errno, yaml +import errno, os, sys, shutil, yaml gProgramName = os.path.split(sys.argv[0])[-1] if sys.version_info < (2, 5, 0): @@ -30,7 +30,6 @@ from gppylib.commands.base import ExecutionError from gppylib.system import configurationInterface, configurationImplGpdb, fileSystemInterface, \ fileSystemImplOs, osInterface, osImplNative, faultProberInterface, faultProberImplGpdb from optparse import OptionGroup, OptionParser, SUPPRESS_HELP -from lockfile.pidlockfile import PIDLockFile, LockTimeout def getProgramName(): @@ -41,6 +40,86 @@ def getProgramName(): global gProgramName return gProgramName +class PIDLockHeld(Exception): + def __init__(self, message, path): + self.message = message + self.path = path + +class PIDLockFile: + """ + Create a lock, utilizing the atomic nature of mkdir on Unix + Inside of this directory, a file named PID contains exactly the PID, with + no newline or space, of the process which created the lock. + + The process which created the lock can release the lock. The lock will + be released by the process which created it on object deletion + """ + + def __init__(self, path): + self.path = path + self.PIDfile = os.path.join(path, "PID") + self.PID = os.getpid() + + def acquire(self): + try: + os.makedirs(self.path) + with open(self.PIDfile, mode='w') as p: + p.write(str(self.PID)) + except EnvironmentError as e: + if e.errno == errno.EEXIST: + raise PIDLockHeld("PIDLock already held at %s" % self.path, self.path) + else: + raise + except: + raise + + def release(self): + """ + If the PIDfile or directory have been removed, the lock no longer + exists, so pass + """ + try: + # only delete the lock if we created the lock + if self.PID == self.read_pid(): + # remove the dir and PID file inside of it + shutil.rmtree(self.path) + except EnvironmentError as e: + if e.errno == errno.ENOENT: + pass + else: + raise + except: + raise + + def read_pid(self): + """ + Return the PID of the process owning the lock as an int + Return None if there is no lock + """ + owner = "" + try: + with open(self.PIDfile) as p: + owner = int(p.read()) + except EnvironmentError as e: + if e.errno == errno.ENOENT: + return None + else: + raise + except: + raise + return owner + + def __enter__(self): + self.acquire() + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + self.release() + return None + + def __del__(self): + self.release() + class SimpleMainLock: """ @@ -56,7 +135,7 @@ class SimpleMainLock: """ def __init__(self, mainOptions): - self.pidfilename = mainOptions.get('pidfilename', None) # the file we're using for locking + self.pidlockpath = mainOptions.get('pidlockpath', None) # the directory we're using for locking self.parentpidvar = mainOptions.get('parentpidvar', None) # environment variable holding parent pid self.parentpid = None # parent pid which already has the lock self.ppath = None # complete path to the lock file @@ -67,8 +146,8 @@ class SimpleMainLock: if self.parentpidvar is not None and self.parentpidvar in os.environ: self.parentpid = int(os.environ[self.parentpidvar]) - if self.pidfilename is not None: - self.ppath = os.path.join(gp.get_masterdatadir(), self.pidfilename) + if self.pidlockpath is not None: + self.ppath = os.path.join(gp.get_masterdatadir(), self.pidlockpath) self.pidlockfile = PIDLockFile(self.ppath) def acquire(self): @@ -91,19 +170,11 @@ class SimpleMainLock: if self.pidfilepid == self.parentpid: return None - # cleanup stale locks - try: - os.kill(self.pidfilepid, signal.SIG_DFL) - except OSError, exc: - if exc.errno == errno.ESRCH: - self.pidlockfile.break_lock() - self.pidfilepid = None - # try and acquire the lock try: - self.pidlockfile.acquire(1) + self.pidlockfile.acquire() - except LockTimeout: + except PIDLockHeld: self.pidfilepid = self.pidlockfile.read_pid() return self.pidfilepid @@ -180,7 +251,7 @@ def simple_main(createOptionParserFn, createCommandFn, mainOptions=None): suppressStartupLogMessage (map to bool) useHelperToolLogging (map to bool) setNonuserOnToolLogger (map to bool, defaults to false) - pidfilename (string) + pidlockpath (string) parentpidvar (string) """ @@ -189,18 +260,19 @@ def simple_main(createOptionParserFn, createCommandFn, mainOptions=None): def simple_main_internal(createOptionParserFn, createCommandFn, mainOptions): """ - If caller specifies 'pidfilename' in mainOptions then we manage the + If caller specifies 'pidlockpath' in mainOptions then we manage the specified pid file within the MASTER_DATA_DIRECTORY before proceeding to execute the specified program and we clean up the pid file when we're done. """ sml = None - if mainOptions is not None and 'pidfilename' in mainOptions: + if mainOptions is not None and 'pidlockpath' in mainOptions: sml = SimpleMainLock(mainOptions) otherpid = sml.acquire() if otherpid is not None: logger = gplog.get_default_logger() - logger.error("An instance of %s is already running (pid %s)" % (getProgramName(), otherpid)) + logger.error("Lockfile %s indicates that an instance of %s is already running with PID %s" % (sml.ppath, getProgramName(), otherpid)) + logger.error("If this is not the case, remove the lockfile directory at %s" % (sml.ppath)) return # at this point we have whatever lock we require diff --git a/gpMgmt/bin/gppylib/programs/clsRecoverSegment.py b/gpMgmt/bin/gppylib/programs/clsRecoverSegment.py index 0fa610b94c..a75401356d 100644 --- a/gpMgmt/bin/gppylib/programs/clsRecoverSegment.py +++ b/gpMgmt/bin/gppylib/programs/clsRecoverSegment.py @@ -789,8 +789,8 @@ class GpRecoverSegmentProgram: def mainOptions(): """ The dictionary this method returns instructs the simple_main framework - to check for a gprecoverseg.pid file under MASTER_DATA_DIRECTORY to + to check for a gprecoverseg.lock file under MASTER_DATA_DIRECTORY to prevent the customer from trying to run more than one instance of gprecoverseg at the same time. """ - return {'pidfilename': 'gprecoverseg.pid', 'parentpidvar': 'GPRECOVERPID'} + return {'pidlockpath': 'gprecoverseg.lock', 'parentpidvar': 'GPRECOVERPID'} diff --git a/gpMgmt/bin/gppylib/test/unit/test_unit_mainUtils.py b/gpMgmt/bin/gppylib/test/unit/test_unit_mainUtils.py new file mode 100755 index 0000000000..5f844a06f9 --- /dev/null +++ b/gpMgmt/bin/gppylib/test/unit/test_unit_mainUtils.py @@ -0,0 +1,80 @@ +import os + +from gp_unittest import * +from gppylib.mainUtils import PIDLockFile, PIDLockHeld + + +class MainUtilsTestCase(GpTestCase): + + def setUp(self): + # parent PID + self.ppid = os.getpid() + self.lockfile = "/tmp/lock-%s" % (str(self.ppid)) + self.lock = PIDLockFile(self.lockfile) + + def test_release_removes_lock(self): + self.lock.acquire() + self.assertEquals(True,os.path.exists(self.lockfile)) + self.lock.release() + self.assertEquals(False, os.path.exists(self.lockfile)) + + def test_with_block_removes_lock(self): + with self.lock: + self.assertEquals(True,os.path.exists(self.lockfile)) + self.assertEquals(False, os.path.exists(self.lockfile)) + + def test_lock_owned_by_parent(self): + with self.lock: + self.assertEquals(l.read_pid(), self.ppid) + + + def test_exceptionPIDLockHeld_if_same_pid(self): + with self.lock: + with self.assertRaises(PIDLockHeld, message="PIDLock already held at %s" % (self.lockfile)): + self.lock.acquire() + + def test_child_can_read_lock_owner(self): + with self.lock: + pid = os.fork() + if pid == 0: + self.assertEquals(l.read_pid(), self.ppid) + os._exit(0) + else: + os.wait() + + def test_exceptionPIDLockHeld_if_different_pid(self): + self.lock.acquire() + pid = os.fork() + # if child, os.fork() == 0 + if pid == 0: + with self.assertRaises(PIDLockHeld, message="PIDLock already held at %s" % (self.lockfile)): + self.lock.acquire() + os._exit(0) + else: + os.wait() + self.lock.release() + + def test_childPID_can_not_remove_parent_lock(self): + with self.lock: + pid = os.fork() + if pid == 0: + newlock = PIDLockFile(self.lockfile) + try: + with newlock: + # the __exit__ or __del__ of this with + # would normally call release - but this + # PID does not own the lock + pass + # we expect the the acquire to fail + except: + pass + self.assertEquals(True, os.path.exists(self.lockfile)) + os._exit(0) + else: + os.wait() + + + + +if __name__ == '__main__': + run_tests() diff --git a/python-dependencies.txt b/python-dependencies.txt index e8b5756270..c5f6c9670d 100644 --- a/python-dependencies.txt +++ b/python-dependencies.txt @@ -1,7 +1,6 @@ argparse==1.2.1 behave==1.2.4 epydoc==3.0.1 -lockfile==0.9.1 logilab-astng==0.20.1 logilab-common==0.50.1 MarkupSafe==1.0 diff --git a/src/tools/docker/centos6/Dockerfile b/src/tools/docker/centos6/Dockerfile index 2ff925e4ae..0ad6ba7dfc 100644 --- a/src/tools/docker/centos6/Dockerfile +++ b/src/tools/docker/centos6/Dockerfile @@ -34,7 +34,7 @@ RUN ssh-keygen -t rsa -N "" -f /root/.ssh/id_rsa && \ # newer version of gcc and run environment for gpdb RUN yum -y install centos-release-scl && \ yum -y install --nogpgcheck devtoolset-7-gcc devtoolset-7-gcc-c++ && yum clean all && \ - pip --no-cache-dir install psi lockfile && \ + pip --no-cache-dir install psi && \ ln -s /usr/bin/cmake3 /usr/bin/cmake && \ echo -e 'source /opt/rh/devtoolset-7/enable' >> /opt/gcc_env.sh && \ echo -e 'source /opt/gcc_env.sh' >> /root/.bashrc && \ diff --git a/src/tools/docker/centos7/Dockerfile b/src/tools/docker/centos7/Dockerfile index 734d552b74..6950bd71a1 100644 --- a/src/tools/docker/centos7/Dockerfile +++ b/src/tools/docker/centos7/Dockerfile @@ -34,7 +34,7 @@ RUN ssh-keygen -t rsa -N "" -f /root/.ssh/id_rsa && \ # newer version of gcc and run environment for gpdb RUN yum -y install centos-release-scl && \ yum -y install --nogpgcheck devtoolset-7-gcc devtoolset-7-gcc-c++ && yum clean all && \ - pip --no-cache-dir install psi lockfile && \ + pip --no-cache-dir install psi && \ ln -s /usr/bin/cmake3 /usr/bin/cmake && \ echo -e 'source /opt/rh/devtoolset-7/enable' >> /opt/gcc_env.sh && \ echo -e 'source /opt/gcc_env.sh' >> /root/.bashrc && \ diff --git a/src/tools/docker/ubuntu/Dockerfile b/src/tools/docker/ubuntu/Dockerfile index c428076370..1827b6b9d3 100644 --- a/src/tools/docker/ubuntu/Dockerfile +++ b/src/tools/docker/ubuntu/Dockerfile @@ -43,7 +43,6 @@ RUN apt-get update -y && \ openssh-server \ pkg-config \ python-dev \ - python-lockfile \ python-pip \ python-psutil \ python-setuptools \ -- GitLab