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: NTyler Ramer <tramer@pivotal.io>

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) ----------------------------
 =================================================================

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

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 <<EOF
 export PS1='\n\w\n$ '
 source /opt/gcc_env.sh

README.ubuntu.bash

@@ -33,7 +33,6 @@ apt-get install -y \
   openssh-server \
   openssl \
   python-dev \
-  python-lockfile \
   python-pip \
   python-psutil \
   python-pygresql \

concourse/scripts/combine_cli_coverage.bash

@@ -86,7 +86,6 @@ omit =
     */python/pygresql/*
     */python/subprocess32.py
     */python/yaml/*
-    */python/lockfile/*
     */gppkg_migrate/*
     */bin/pythonSrc/ext/*
 EOF

gpMgmt/Makefile

@@ -18,9 +18,6 @@ install: generate_greenplum_path_file
 	if [ -e bin/ext/__init__.py ]; then \
 	    cp -rp bin/ext/__init__.py $(DESTDIR)$(prefix)/lib/python ; \
 	fi
-	if [ -e bin/ext/lockfile ]; then \
-	    cp -rp bin/ext/lockfile $(DESTDIR)$(prefix)/lib/python ; \
-	fi
 	if [ -e bin/ext/psutil ]; then \
 	    cp -rp bin/ext/psutil $(DESTDIR)$(prefix)/lib/python ; \
 	fi

gpMgmt/bin/Makefile

@@ -59,7 +59,7 @@ core: subprocess32
 	python gpconfig_modules/parse_guc_metadata.py $(DESTDIR)$(prefix)
 
 ifneq "$(wildcard $(CURDIR)/pythonSrc/ext/*.tar.gz)" ""
-install: installdirs installprograms core lockfile psutil pygresql
+install: installdirs installprograms core psutil pygresql
 else
 install: installdirs installprograms core
 endif
@@ -79,27 +79,9 @@ pygresql:
 	cd $(PYLIB_SRC_EXT)/$(PYGRESQL_DIR)/ && PATH=$(bindir):$$PATH python setup.py build
 	cp -r $(PYLIB_SRC_EXT)/$(PYGRESQL_DIR)/build/lib*/* $(PYLIB_DIR)/
 
-
-
 #
-# LOCKFILE
+# subprocess32
 #
-# note the awk commands are used to eliminate references to code in __init__.py
-# that we don't use and also to insert a comment to tell pylint not to complain
-# about these files since we are not in a position to correct those warnings.
-#
-LOCKFILE_VERSION=0.9.1
-LOCKFILE_DIR=lockfile-$(LOCKFILE_VERSION)
-LOCKFILE_SRC=$(PYLIB_SRC_EXT)/$(LOCKFILE_DIR)/build/lib*/lockfile
-LOCKFILE_DST=$(PYLIB_DIR)/lockfile
-lockfile:
-	@echo "--- lockfile"
-	cd $(PYLIB_SRC_EXT)/ && $(TAR) xzf $(LOCKFILE_DIR).tar.gz
-	cd $(PYLIB_SRC_EXT)/$(LOCKFILE_DIR)/ && python setup.py build
-	mkdir -p $(LOCKFILE_DST)
-	awk 'BEGIN{print "# pylint: disable-all"} /^if hasattr/ {exit} { print }' < `echo $(LOCKFILE_SRC)/__init__.py`    > $(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 \

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")

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

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'}

gpMgmt/bin/gppylib/test/unit/test_unit_mainUtils.py

+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()

python-dependencies.txt

 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

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 && \

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 && \

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 \