geo-rep / gsyncd: fix cleanup of temporary mounts

[This is a "forward port" of fafd5c17, http://review.gluster.com/2908]

The "finally" clause that was meant to cleanup after the
temp mount has not covered the case of getting signalled
(eg. by monitor, upon worker timing out).

So here we "outsource" the cleanup to an ephemeral child process.
Child calls setsid(2) so it won't be bothered by internal process
management. We use a pipe in between worker and the cleanup child;
when child sees the worker end getting closed, it performs the cleanup.
Worker end can get closed either because worker closes it (normal case),
or because worker has terminated (faulty case) -- thus as bonus, we get
a nice uniform handling with no need to differentiate between normal and
faulty cases.

The faulty case that was seen IRL -- ie., users of maintainance mounts
hang in chdir(2) to mount point -- can be simulated for testing purposes
by applying the following patch:

  diff --git a/xlators/mount/fuse/src/fuse-bridge.c b/xlators/mount/fuse/src/fuse-bridge.c
  index acd3c68..1ce5dc1 100644
  --- a/xlators/mount/fuse/src/fuse-bridge.c
  +++ b/xlators/mount/fuse/src/fuse-bridge.c
  @@ -2918,7 +2918,7 @@ fuse_init (xlator_t *this, fuse_in_header_t *finh, void *msg)
           if (fini->minor < 9)
                   *priv->msg0_len_p = sizeof(*finh) + FUSE_COMPAT_WRITE_IN_SIZE;
   #endif
  -        ret = send_fuse_obj (this, finh, &fino);
  +        ret = priv->client_pid_set ? 0 : send_fuse_obj (this, finh, &fino);
           if (ret == 0)
                   gf_log ("glusterfs-fuse", GF_LOG_INFO,
                           "FUSE inited with protocol versions:"

Change-Id: I14bad56a60a7fa82d0104fa4b9a20f4e42a7186f
BUG: 786291
Signed-off-by: Csaba Henk <csaba@redhat.com>
Reviewed-on: http://review.gluster.com/3259
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Jeff Darcy <jdarcy@redhat.com>
Reviewed-by: Anand Avati <avati@redhat.com>

1 files changed, 76 insertions, 17 deletions

diff --git a/xlators/features/marker/utils/syncdaemon/resource.py b/xlators/features/marker/utils/syncdaemon/resource.py
index 923cf78d4cd..bb7dca61d47 100644
--- a/xlators/features/marker/utils/syncdaemon/resource.py
+++ b/xlators/features/marker/utils/syncdaemon/resource.py
@@ -3,6 +3,7 @@ import os
 import sys
 import stat
 import time
+import fcntl
 import errno
 import struct
 import socket
@@ -149,6 +150,14 @@ class Popen(subprocess.Popen):
         t.start()
         cls.errhandler = t
 
+    @classmethod
+    def fork(cls):
+        """fork wrapper that restarts errhandler thread in child"""
+        pid = os.fork()
+        if not pid:
+            cls.init_errhandler()
+        return pid
+
     def __init__(self, args, *a, **kw):
         """customizations for subprocess.Popen instantiation
 
@@ -172,8 +181,8 @@ class Popen(subprocess.Popen):
             assert(getattr(self, 'errhandler', None))
             self.errstore[self] = []
 
-    def errfail(self):
-        """fail nicely if child did not terminate with success"""
+    def errlog(self):
+        """make a log about child's failure event"""
         filling = ""
         if self.elines:
             filling = ", saying:"
@@ -182,6 +191,10 @@ class Popen(subprocess.Popen):
         for l in self.elines:
             for ll in l.rstrip().split("\n"):
                 logging.error(self.args[0] + "> " + ll.rstrip())
+
+    def errfail(self):
+        """fail nicely if child did not terminate with success"""
+        self.errlog()
         syncdutils.finalize(exval = 1)
 
     def terminate_geterr(self, fail_on_err = True):
@@ -626,6 +639,7 @@ class GLUSTER(AbstractUrl, SlaveLocal, SlaveRemote):
 
         def __init__(self, params):
             self.params = params
+            self.mntpt = None
 
         @classmethod
         def get_glusterprog(cls):
@@ -644,7 +658,7 @@ class GLUSTER(AbstractUrl, SlaveLocal, SlaveRemote):
         def make_mount_argv(self, *a):
             raise NotImplementedError
 
-        def cleanup_mntpt(self):
+        def cleanup_mntpt(self, *a):
             pass
 
         def handle_mounter(self, po):
@@ -657,24 +671,67 @@ class GLUSTER(AbstractUrl, SlaveLocal, SlaveRemote):
             change into the mount, and lazy unmount the
             filesystem.
             """
-            mounted = False
-            try:
-                po = Popen(self.make_mount_argv(*a), **self.mountkw)
+
+            mpi, mpo = os.pipe()
+            mh = Popen.fork()
+            if mh:
+                os.close(mpi)
+                fcntl.fcntl(mpo, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
+                d = None
+                margv = self.make_mount_argv(*a)
+                if self.mntpt:
+                    # mntpt is determined pre-mount
+                    d = self.mntpt
+                    os.write(mpo, d + '\0')
+                po = Popen(margv, **self.mountkw)
                 self.handle_mounter(po)
                 po.terminate_geterr()
-                d = self.mntpt
-                mounted = True
                 logging.debug('auxiliary glusterfs mount in place')
+                if not d:
+                    # mntpt is determined during mount
+                    d = self.mntpt
+                    os.write(mpo, d + '\0')
+                os.write(mpo, 'M')
                 os.chdir(d)
-                self.umount_l(d).terminate_geterr()
-                mounted = False
-            finally:
+                os.close(mpo)
+                _, rv = syncdutils.waitpid(mh, 0)
+                if rv:
+                    rv = (os.WIFEXITED(rv) and os.WEXITSTATUS(rv) or 0) - \
+                         (os.WIFSIGNALED(rv) and os.WTERMSIG(rv) or 0)
+                    logging.warn('stale mount possibly left behind on ' + d)
+                    raise GsyncdError("cleaning up temp mountpoint %s failed with status %d" % \
+                                      (d, rv))
+            else:
+                rv = 0
                 try:
-                    if mounted:
-                        self.umount_l(d).terminate_geterr(fail_on_err = False)
-                    self.cleanup_mntpt()
+                    os.setsid()
+                    os.close(mpo)
+                    mntdata = ''
+                    while True:
+                        c = os.read(mpi, 1)
+                        if not c:
+                            break
+                        mntdata += c
+                    if mntdata:
+                        mounted = False
+                        if mntdata[-1] == 'M':
+                            mntdata = mntdata[:-1]
+                            assert(mntdata)
+                            mounted = True
+                        assert(mntdata[-1] == '\0')
+                        mntpt = mntdata[:-1]
+                        assert(mntpt)
+                        if mounted:
+                            po = self.umount_l(mntpt)
+                            po.terminate_geterr(fail_on_err = False)
+                            if po.returncode != 0:
+                                po.errlog()
+                                rv = po.returncode
+                        self.cleanup_mntpt(mntpt)
                 except:
-                    logging.warn('stale mount possibly left behind on ' + d)
+                    logging.exception('mount cleanup failure:')
+                    rv = 200
+                os._exit(rv)
             logging.debug('auxiliary glusterfs mount prepared')
 
     class DirectMounter(Mounter):
@@ -691,8 +748,10 @@ class GLUSTER(AbstractUrl, SlaveLocal, SlaveRemote):
             self.mntpt = tempfile.mkdtemp(prefix = 'gsyncd-aux-mount-')
             return [self.get_glusterprog()] + ['--' + p for p in self.params] + [self.mntpt]
 
-        def cleanup_mntpt(self):
-            os.rmdir(self.mntpt)
+        def cleanup_mntpt(self, mntpt = None):
+            if not mntpt:
+                mntpt = self.mntpt
+            os.rmdir(mntpt)
 
     class MountbrokerMounter(Mounter):
         """mounter backend using the mountbroker gluster service"""