[PATCH] mock _init _umountall

[Paul B Schroeder]

Hello all...

Recently, I was doing some work which needed a more extensive /dev than the mock internal dev setup.  So I tried doing this in site-defaults.cfg:
config_opts['internal_dev_setup'] = False
config_opts['plugin_conf']['bind_mount_enable'] = True
config_opts['plugin_conf']['bind_mount_opts']['dirs'].append(('/dev', '/dev' ))

And discovered that mock chroot init was failing due _umountall getting called (in _init) before _makeBuildUser.  In _makeBuildUser there is a perl command which, for whatever reason, would fail due to /dev/null not being available at that point.

At any rate, the following patch seems to take care of the issue.  And it would seem to be a better way to handle this in general as I would expect _umountall to be the last method called.

It would be awesome if this or something similar could be applied..

Thanks...Paul...

diff --git a/py/mock/backend.py b/py/mock/backend.py
index ca06f59..9df438b 100644
--- a/py/mock/backend.py
+++ b/py/mock/backend.py
@@ -282,24 +282,24 @@ class Root(object):
             self._mountall()
             if self.chrootWasCleaned:
                 self._yum(self.chroot_setup_cmd, returnOutput=1)
-        finally:
-            self._umountall()

-        # create user
-        self._makeBuildUser()
+            # create user
+            self._makeBuildUser()

-        # create rpmbuild dir
-        self._buildDirSetup()
+            # create rpmbuild dir
+            self._buildDirSetup()

-        # set up timezone to match host
-        localtimedir = self.makeChrootPath('etc')
-        localtimepath = self.makeChrootPath('etc', 'localtime')
-        if os.path.exists(localtimepath):
-            os.remove(localtimepath)
-        shutil.copy2('/etc/localtime', localtimedir)
+            # set up timezone to match host
+            localtimedir = self.makeChrootPath('etc')
+            localtimepath = self.makeChrootPath('etc', 'localtime')
+            if os.path.exists(localtimepath):
+                os.remove(localtimepath)
+            shutil.copy2('/etc/localtime', localtimedir)

-        # done with init
-        self._callHooks('postinit')
+            # done with init
+            self._callHooks('postinit')
+        finally:
+            self._umountall()

decorate(traceLog())
     def _setupDev(self):

[Paul B Schroeder]

Here is an updated patch.  I found that _umountall was unmounting in the same order of the mounts and this was also causing some issues.  _umountall now unwinds the mounts by executing the umount commands in reverse order of the mounts.
I also went ahead an created a trac ticket:  https://fedorahosted.org/mock/ticket/8

Cheers...Paul...

diff --git a/py/mock/backend.py b/py/mock/backend.py
index ca06f59..bad33b5 100644
--- a/py/mock/backend.py
+++ b/py/mock/backend.py
@@ -282,24 +282,24 @@ class Root(object):
             self._mountall()
             if self.chrootWasCleaned:
                 self._yum(self.chroot_setup_cmd, returnOutput=1)
-        finally:
-            self._umountall()

-        # create user
-        self._makeBuildUser()
+            # create user
+            self._makeBuildUser()

-        # create rpmbuild dir
-        self._buildDirSetup()
+            # create rpmbuild dir
+            self._buildDirSetup()

-        # set up timezone to match host
-        localtimedir = self.makeChrootPath('etc')
-        localtimepath = self.makeChrootPath('etc', 'localtime')
-        if os.path.exists(localtimepath):
-            os.remove(localtimepath)
-        shutil.copy2('/etc/localtime', localtimedir)
+            # set up timezone to match host
+            localtimedir = self.makeChrootPath('etc')
+            localtimepath = self.makeChrootPath('etc', 'localtime')
+            if os.path.exists(localtimepath):
+                os.remove(localtimepath)
+            shutil.copy2('/etc/localtime', localtimedir)

-        # done with init
-        self._callHooks('postinit')
+            # done with init
+            self._callHooks('postinit')
+        finally:
+            self._umountall()

decorate(traceLog())
     def _setupDev(self):
@@ -589,7 +589,10 @@ class Root(object):
     decorate(traceLog())
     def _umountall(self):
         """umount all mounted chroot fs."""
-        for cmd in self.umountCmds:
+        # Unwind mounts by umounting in the opposite order of the mounts
+        umountCmds = self.umountCmds
+        umountCmds.reverse()
+        for cmd in umountCmds:
             self.root_log.debug(cmd)
             mock.util.do(cmd, raiseExc=0, shell=True)

On 06/25/2010 03:11 PM, Paul B Schroeder wrote:
> Hello all...
>
> Recently, I was doing some work which needed a more extensive /dev than the mock internal dev setup.  So I tried doing this in site-defaults.cfg:
> config_opts['internal_dev_setup'] = False
> config_opts['plugin_conf']['bind_mount_enable'] = True
> config_opts['plugin_conf']['bind_mount_opts']['dirs'].append(('/dev', '/dev' ))
>
> And discovered that mock chroot init was failing due _umountall getting called (in _init) before _makeBuildUser.  In _makeBuildUser there is a perl command which, for whatever reason, would fail due to /dev/null not being available at that point.
>
> At any rate, the following patch seems to take care of the issue.  And it would seem to be a better way to handle this in general as I would expect _umountall to be the last method called.
>
> It would be awesome if this or something similar could be applied..
>
> Thanks...Paul...
>
> diff --git a/py/mock/backend.py b/py/mock/backend.py
> index ca06f59..9df438b 100644
> --- a/py/mock/backend.py
> +++ b/py/mock/backend.py
> @@ -282,24 +282,24 @@ class Root(object):
>               self._mountall()
>               if self.chrootWasCleaned:
>                   self._yum(self.chroot_setup_cmd, returnOutput=1)
>    finally:
>        self._umountall()
>
>    # create user
>    self._makeBuildUser()
>        # create user
>        self._makeBuildUser()
>
>    # create rpmbuild dir
>    self._buildDirSetup()
>        # create rpmbuild dir
>        self._buildDirSetup()
>
>    # set up timezone to match host
>    localtimedir = self.makeChrootPath('etc')
>    localtimepath = self.makeChrootPath('etc', 'localtime')
>    if os.path.exists(localtimepath):
>        os.remove(localtimepath)
>    shutil.copy2('/etc/localtime', localtimedir)
>        # set up timezone to match host
>        localtimedir = self.makeChrootPath('etc')
>        localtimepath = self.makeChrootPath('etc', 'localtime')
>        if os.path.exists(localtimepath):
>            os.remove(localtimepath)
>        shutil.copy2('/etc/localtime', localtimedir)
>
>    # done with init
>    self._callHooks('postinit')
>        # done with init
>        self._callHooks('postinit')
>    finally:
>        self._umountall()
>
> decorate(traceLog())
> def _setupDev(self):

[Clark Williams]

On Wed, 30 Jun 2010 16:10:20 -0500
Paul B Schroeder  wrote:
> Here is an updated patch.  I found that _umountall was unmounting in the same order of the mounts and this was also causing some issues.  _umountall now unwinds the mounts by executing the umount commands in reverse order of the mounts.
> I also went ahead an created a trac ticket:  https://fedorahosted.org/mock/ticket/8
>
> Cheers...Paul...

Thanks Paul, just wanted to let you know that we're not *completely*
ignoring you, just busy with release stuff. I'll merge this (or a
tweaked version of it) and get it out probably just after the 4th.

Clark
> diff --git a/py/mock/backend.py b/py/mock/backend.py
> index ca06f59..bad33b5 100644
> --- a/py/mock/backend.py
> +++ b/py/mock/backend.py
> @@ -282,24 +282,24 @@ class Root(object):
>              self._mountall()
>              if self.chrootWasCleaned:
>                  self._yum(self.chroot_setup_cmd, returnOutput=1)
>    finally:
>        self._umountall()
>
>    # create user
>    self._makeBuildUser()
>        # create user
>        self._makeBuildUser()
>
>    # create rpmbuild dir
>    self._buildDirSetup()
>        # create rpmbuild dir
>        self._buildDirSetup()
>
>    # set up timezone to match host
>    localtimedir = self.makeChrootPath('etc')
>    localtimepath = self.makeChrootPath('etc', 'localtime')
>    if os.path.exists(localtimepath):
>        os.remove(localtimepath)
>    shutil.copy2('/etc/localtime', localtimedir)
>        # set up timezone to match host
>        localtimedir = self.makeChrootPath('etc')
>        localtimepath = self.makeChrootPath('etc', 'localtime')
>        if os.path.exists(localtimepath):
>            os.remove(localtimepath)
>        shutil.copy2('/etc/localtime', localtimedir)
>
>    # done with init
>    self._callHooks('postinit')
>        # done with init
>        self._callHooks('postinit')
>    finally:
>        self._umountall()
>
> decorate(traceLog())
>    def _setupDev(self):
>
> @@ -589,7 +589,10 @@ class Root(object):
>      decorate(traceLog())
>      def _umountall(self):
>          """umount all mounted chroot fs."""
>    for cmd in self.umountCmds:
>    # Unwind mounts by umounting in the opposite order of the mounts
>    umountCmds = self.umountCmds
>    umountCmds.reverse()
>    for cmd in umountCmds:
>        self.root_log.debug(cmd)
>        mock.util.do(cmd, raiseExc=0, shell=True)
>
>
>
> On 06/25/2010 03:11 PM, Paul B Schroeder wrote:
> > Hello all...
> >
> > Recently, I was doing some work which needed a more extensive /dev than the mock internal dev setup.  So I tried doing this in site-defaults.cfg:
> > config_opts['internal_dev_setup'] = False
> > config_opts['plugin_conf']['bind_mount_enable'] = True
> > config_opts['plugin_conf']['bind_mount_opts']['dirs'].append(('/dev', '/dev' ))
> >
> > And discovered that mock chroot init was failing due _umountall getting called (in _init) before _makeBuildUser.  In _makeBuildUser there is a perl command which, for whatever reason, would fail due to /dev/null not being available at that point.
> >
> > At any rate, the following patch seems to take care of the issue.  And it would seem to be a better way to handle this in general as I would expect _umountall to be the last method called.
> >
> > It would be awesome if this or something similar could be applied..
> >
> > Thanks...Paul...
> >
> > diff --git a/py/mock/backend.py b/py/mock/backend.py
> > index ca06f59..9df438b 100644
> > --- a/py/mock/backend.py
> > +++ b/py/mock/backend.py
> > @@ -282,24 +282,24 @@ class Root(object):
> >               self._mountall()
> >               if self.chrootWasCleaned:
> >                   self._yum(self.chroot_setup_cmd, returnOutput=1)
> >    finally:
> >        self._umountall()
> >
> >    # create user
> >    self._makeBuildUser()
> >        # create user
> >        self._makeBuildUser()
> >
> >    # create rpmbuild dir
> >    self._buildDirSetup()
> >        # create rpmbuild dir
> >        self._buildDirSetup()
> >
> >    # set up timezone to match host
> >    localtimedir = self.makeChrootPath('etc')
> >    localtimepath = self.makeChrootPath('etc', 'localtime')
> >    if os.path.exists(localtimepath):
> >        os.remove(localtimepath)
> >    shutil.copy2('/etc/localtime', localtimedir)
> >        # set up timezone to match host
> >        localtimedir = self.makeChrootPath('etc')
> >        localtimepath = self.makeChrootPath('etc', 'localtime')
> >        if os.path.exists(localtimepath):
> >            os.remove(localtimepath)
> >        shutil.copy2('/etc/localtime', localtimedir)
> >
> >    # done with init
> >    self._callHooks('postinit')
> >        # done with init
> >        self._callHooks('postinit')
> >    finally:
> >        self._umountall()
> >
> > decorate(traceLog())
> > def _setupDev(self):
> --
>
> Paul B Schroeder
> <paulbsch "at" vbridges "dot" com>
>
> buildsys mailing list
> [e-mail]
> https://admin.fedoraproject.org/mailman/listinfo/buildsys

[Paul B Schroeder]

On 07/01/2010 10:55 AM, Clark Williams wrote:
> On Wed, 30 Jun 2010 16:10:20 -0500
> Paul B Schroeder  wrote:
> > Here is an updated patch.  I found that _umountall was unmounting in the same order of the mounts and this was also causing some issues.  _umountall now unwinds the mounts by executing the umount commands in reverse order of the mounts.
> > I also went ahead an created a trac ticket:  https://fedorahosted.org/mock/ticket/8
> >
> > Cheers...Paul...
>
> Thanks Paul, just wanted to let you know that we're not *completely*
> ignoring you, just busy with release stuff. I'll merge this (or a
> tweaked version of it) and get it out probably just after the 4th.
>
> Clark

Great..  Thanks..  You probably noticed already, but just in case you 
didn't, I also provided a patch to implement the --scrub option we 
talked about way back when:
https://bugzilla.redhat.com/show_bug.cgi?id=450726

Cheers...Paul...
> > diff --git a/py/mock/backend.py b/py/mock/backend.py
> > index ca06f59..bad33b5 100644
> > --- a/py/mock/backend.py
> > +++ b/py/mock/backend.py
> > @@ -282,24 +282,24 @@ class Root(object):
> >               self._mountall()
> >               if self.chrootWasCleaned:
> >                   self._yum(self.chroot_setup_cmd, returnOutput=1)
> >    finally:
> >        self._umountall()
> >
> >    # create user
> >    self._makeBuildUser()
> >        # create user
> >        self._makeBuildUser()
> >
> >    # create rpmbuild dir
> >    self._buildDirSetup()
> >        # create rpmbuild dir
> >        self._buildDirSetup()
> >
> >    # set up timezone to match host
> >    localtimedir = self.makeChrootPath('etc')
> >    localtimepath = self.makeChrootPath('etc', 'localtime')
> >    if os.path.exists(localtimepath):
> >        os.remove(localtimepath)
> >    shutil.copy2('/etc/localtime', localtimedir)
> >        # set up timezone to match host
> >        localtimedir = self.makeChrootPath('etc')
> >        localtimepath = self.makeChrootPath('etc', 'localtime')
> >        if os.path.exists(localtimepath):
> >            os.remove(localtimepath)
> >        shutil.copy2('/etc/localtime', localtimedir)
> >
> >    # done with init
> >    self._callHooks('postinit')
> >        # done with init
> >        self._callHooks('postinit')
> >    finally:
> >        self._umountall()
> >
> > decorate(traceLog())
> > def _setupDev(self):
> >
> > @@ -589,7 +589,10 @@ class Root(object):
> >       decorate(traceLog())
> >       def _umountall(self):
> >           """umount all mounted chroot fs."""
> >    for cmd in self.umountCmds:
> >    # Unwind mounts by umounting in the opposite order of the mounts
> >    umountCmds = self.umountCmds
> >    umountCmds.reverse()
> >    for cmd in umountCmds:
> >         self.root_log.debug(cmd)
> >         mock.util.do(cmd, raiseExc=0, shell=True)
> >
> >
> >
> > On 06/25/2010 03:11 PM, Paul B Schroeder wrote:
> > > Hello all...
> > >
> > > Recently, I was doing some work which needed a more extensive /dev than the mock internal dev setup.  So I tried doing this in site-defaults.cfg:
> > > config_opts['internal_dev_setup'] = False
> > > config_opts['plugin_conf']['bind_mount_enable'] = True
> > > config_opts['plugin_conf']['bind_mount_opts']['dirs'].append(('/dev', '/dev' ))
> > >
> > > And discovered that mock chroot init was failing due _umountall getting called (in _init) before _makeBuildUser.  In _makeBuildUser there is a perl command which, for whatever reason, would fail due to /dev/null not being available at that point.
> > >
> > > At any rate, the following patch seems to take care of the issue.  And it would seem to be a better way to handle this in general as I would expect _umountall to be the last method called.
> > >
> > > It would be awesome if this or something similar could be applied..
> > >
> > > Thanks...Paul...
> > >
> > > diff --git a/py/mock/backend.py b/py/mock/backend.py
> > > index ca06f59..9df438b 100644
> > > --- a/py/mock/backend.py
> > > +++ b/py/mock/backend.py
> > > @@ -282,24 +282,24 @@ class Root(object):
> > >                self._mountall()
> > >                if self.chrootWasCleaned:
> > >                    self._yum(self.chroot_setup_cmd, returnOutput=1)
> > >    finally:
> > >        self._umountall()
> > >
> > >    # create user
> > >    self._makeBuildUser()
> > >        # create user
> > >        self._makeBuildUser()
> > >
> > >    # create rpmbuild dir
> > >    self._buildDirSetup()
> > >        # create rpmbuild dir
> > >        self._buildDirSetup()
> > >
> > >    # set up timezone to match host
> > >    localtimedir = self.makeChrootPath('etc')
> > >    localtimepath = self.makeChrootPath('etc', 'localtime')
> > >    if os.path.exists(localtimepath):
> > >        os.remove(localtimepath)
> > >    shutil.copy2('/etc/localtime', localtimedir)
> > >        # set up timezone to match host
> > >        localtimedir = self.makeChrootPath('etc')
> > >        localtimepath = self.makeChrootPath('etc', 'localtime')
> > >        if os.path.exists(localtimepath):
> > >            os.remove(localtimepath)
> > >        shutil.copy2('/etc/localtime', localtimedir)
> > >
> > >    # done with init
> > >    self._callHooks('postinit')
> > >        # done with init
> > >        self._callHooks('postinit')
> > >    finally:
> > >        self._umountall()
> > >
> > >  decorate(traceLog())
> > >  def _setupDev(self):
> > --
> >
> > Paul B Schroeder
> > <paulbsch "at" vbridges "dot" com>
> >
> > buildsys mailing list
> > [e-mail]
> > https://admin.fedoraproject.org/mailman/listinfo/buildsys

[Clark Williams]

On Thu, 08 Jul 2010 11:18:05 -0500
Paul B Schroeder  wrote:
> On 07/01/2010 10:55 AM, Clark Williams wrote:
> > On Wed, 30 Jun 2010 16:10:20 -0500
> > Paul B Schroeder  wrote:
> > > Here is an updated patch.  I found that _umountall was unmounting in the same order of the mounts and this was also causing some issues.  _umountall now unwinds the mounts by executing the umount commands in reverse order of the mounts.
> > > I also went ahead an created a trac ticket:  https://fedorahosted.org/mock/ticket/8
> > >
> > > Cheers...Paul...
> >
> > Thanks Paul, just wanted to let you know that we're not *completely*
> > ignoring you, just busy with release stuff. I'll merge this (or a
> > tweaked version of it) and get it out probably just after the 4th.
> >
> > Clark
>
> Great..  Thanks..  You probably noticed already, but just in case you 
> didn't, I also provided a patch to implement the --scrub option we 
> talked about way back when:
> https://bugzilla.redhat.com/show_bug.cgi?id=450726
>
> Cheers...Paul...

Yup, I did see it and greatly appreciate it.

Clark