Merge lp:~danilo/vmbuilder/bug-536940 into lp:vmbuilder

[Данило Шеган]

(Re-)Add --tmpfs and --tmp options to vmbuilder.

= Bug 536940 =

So, this re-introduces a --tmpfs and --tmp options. I'm not entirely sure of the syntax for --tmpfs, so I implemented what seems to have been the idea according to the documentation and old web site snippets I found. I'd probably prefer it if it was two options ("--use-tmpfs" and "--tmpfs-size"), but I don't have the time to implement that.

I've also simplified util.tmpfile and util.tmpdir functions: they were always called with the same value of 'keep' parameter. I'd be either leaning on removing them altogether, or making them useful for something (like providing a standard vmbuilder prefix like "vmbuilder" :): they are now single-line calls to tempfile module, and we should just use it directly then.

I've done a few more cleanups of my own code as well since I first written it up: I've removed catching of VMBuilderUserError exception because a run_cmd exception will contain much more useful information than my error message in util.set_up_tmpfs and util.clean_up_tmpfs.

There is also one thing I am unsure about: I'm unmounting a tmpfs even when an error occurs, but that means that you can't go into a chroot and figure out what went wrong. I found that more useful (and when I did hit a problem that needed me to go into chroot directly, and wasn't just an error on my part, I simply ran it without --tmpfs option), but I can easily be convinced the opposite.

FWIW, that sys.exit(1) call is removed simply because it's never executed with a raise just before it.

Note: I won't have much time to work on this. For instance, to actually make tmpfile calls safer, we'd at least do the following: actually create a file using tempfile.mkstemp(), and remove it only just prior to doing the operation which is going to write to that file. That wouldn't remove the risks, would just make them much, much smaller. Of course, I understand that vmbuilder is not designed to be run on machines with untrusted users, so even if I had time to spend on it, I don't think it'd be worthwhile.

[lp:~danilo/vmbuilder/bug-536940]

455. By Данило Шеган on 2010-06-10

Don't try to rm -rf a mounted tmpfs.

[Soren Hansen (soren)]

On Thu, Jun 10, 2010 at 05:22:23PM -0000, Данило Шеган wrote:
> I've also simplified util.tmpfile and util.tmpdir functions: they were
> always called with the same value of 'keep' parameter.

Ah, good catch. This probably hasn't been the case all along, but I
can't think of an easy way to discover that a particular interface is no
longer used. :(

> I'd be either leaning on removing them altogether, or making them
> useful for something (like providing a standard vmbuilder prefix like
> "vmbuilder" :): they are now single-line calls to tempfile module, and
> we should just use it directly then.

I'm in favour of the vmbuilder prefix idea. It would make cleaning up
after VMBuilder during development easier as well.

> There is also one thing I am unsure about: I'm unmounting a tmpfs even
> when an error occurs, but that means that you can't go into a chroot
> and figure out what went wrong. I found that more useful (and when I
> did hit a problem that needed me to go into chroot directly, and
> wasn't just an error on my part, I simply ran it without --tmpfs
> option), but I can easily be convinced the opposite.

Historically, VMBuilder would clean up very well after itself. This was
slightly annoying during development (as you've identified), and it also
had some unfortunate side effects (it would delete people's /dev (in the
host) in some cases). In 0.12.x a lot of these cleanup procedures are
simply not run at all. What I would like, really, is to be able to pass
a --development option or set an environment variable or something that
would make VMBuilder not clean up after itself at all, but for
end-users, even in case of errors, it would clean up.

> Note: I won't have much time to work on this. For instance, to
> actually make tmpfile calls safer, we'd at least do the following:
> actually create a file using tempfile.mkstemp(), and remove it only
> just prior to doing the operation which is going to write to that
> file. That wouldn't remove the risks, would just make them much, much
> smaller.

Alternatively, we could create a temporary directory with mkdtemp with
mode 0700 and use that as the base directory for all the subsequent
temporary files. That way we avoid the race condition, because there's
simply no way another user could a file in there.

> Of course, I understand that vmbuilder is not designed to be run on
> machines with untrusted users, so even if I had time to spend on it, I
> don't think it'd be worthwhile.

Well, it's not meant to make any assumptions ot the opposite either, so
if someone wants to take a stab at this, that would be great.

The patch looks fine. Thanks!

  review +1
  merge approved

--
Soren Hansen
Ubuntu Developer
http://www.ubuntu.com/

review: Approve

=== modified file 'VMBuilder/contrib/cli.py'
--- VMBuilder/contrib/cli.py	2010-04-07 19:47:58 +0000
+++ VMBuilder/contrib/cli.py	2010-06-10 17:56:24 +0000
@@ -22,6 +22,7 @@
 import pwd
 import shutil
 import sys
+import tempfile
 import VMBuilder
 import VMBuilder.util as util
 from   VMBuilder.disk import parse_size
@@ -32,6 +33,7 @@
     arg = 'cli'
 
     def main(self):
+        tmpfs_mount_point = None
         try:
             optparser = optparse.OptionParser()
 
@@ -49,6 +51,19 @@
             group.add_option('--destdir', '-d', type='str', help='Destination directory')
             group.add_option('--only-chroot', action='store_true', help="Only build the chroot. Don't install it on disk images or anything.")
             group.add_option('--existing-chroot', help="Use existing chroot.")
+            group.add_option(
+                '--tmp', '-t', metavar='DIR', dest='tmp_root',
+                default=tempfile.gettempdir(),
+                help=(
+                    'Use TMP as temporary working space for image generation. '
+                    'Defaults to $TMPDIR if it is defined or /tmp otherwise. '
+                    '[default: %default]'))
+            group.add_option(
+                '--tmpfs', metavar="SIZE",
+                help=(
+                    'Use a tmpfs as the working directory, specifying '
+                    'its size or "-" to use tmpfs default '
+                    '(suid,dev,size=1G).'))
             optparser.add_option_group(group)
 
             group = optparse.OptionGroup(optparser, 'Disk')
@@ -105,7 +120,16 @@
                 distro.set_chroot_dir(self.options.existing_chroot)
                 distro.call_hooks('preflight_check')
             else:
-                chroot_dir = util.tmpdir()
+                if self.options.tmpfs is not None:
+                    if str(self.options.tmpfs) == '-':
+                        tmpfs_size = 1024
+                    else:
+                        tmpfs_size = int(self.options.tmpfs)
+                    tmpfs_mount_point = util.set_up_tmpfs(
+                        tmp_root=self.options.tmp_root, size=tmpfs_size)
+                    chroot_dir = tmpfs_mount_point
+                else:
+                    chroot_dir = util.tmpdir(tmp_root=self.options.tmp_root)
                 distro.set_chroot_dir(chroot_dir)
                 distro.build_chroot()
 
@@ -123,12 +147,15 @@
             # and if we reach here, it means the user didn't pass
             # --only-chroot. Hence, we need to remove it to clean
             # up after ourselves.
-            if chroot_dir:
+            if chroot_dir is not None and tmpfs_mount_point is None:
                 util.run_cmd('rm', '-rf', '--one-file-system', chroot_dir)
         except VMBuilderException, e:
             logging.error(e)
             raise
-            sys.exit(1)
+        finally:
+            if tmpfs_mount_point is not None:
+                util.clean_up_tmpfs(tmpfs_mount_point)
+                util.run_cmd('rmdir', tmpfs_mount_point)
 
     def fix_ownership(self, filename):
         """
@@ -195,19 +222,19 @@
             swapsize = parse_size(self.options.swapsize)
             optsize = parse_size(self.options.optsize)
             if hypervisor.preferred_storage == VMBuilder.hypervisor.STORAGE_FS_IMAGE:
-                tmpfile = util.tmpfile(keep=False)
+                tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
                 hypervisor.add_filesystem(filename=tmpfile, size='%dM' % rootsize, type='ext3', mntpnt='/')
-                tmpfile = util.tmpfile(keep=False)
+                tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
                 hypervisor.add_filesystem(filename=tmpfile, size='%dM' % swapsize, type='swap', mntpnt=None)
                 if optsize > 0:
-                    tmpfile = util.tmpfile(keep=False)
+                    tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
                     hypervisor.add_filesystem(filename=tmpfile, size='%dM' % optsize, type='ext3', mntpnt='/opt')
             else:
                 if self.options.raw:
                     disk = hypervisor.add_disk(filename=self.options.raw)
                 else:
                     size = rootsize + swapsize + optsize
-                    tmpfile = util.tmpfile(keep=False)
+                    tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
                     disk = hypervisor.add_disk(tmpfile, size='%dM' % size)
                 offset = 0
                 disk.add_part(offset, rootsize, default_filesystem, '/')
@@ -222,7 +249,8 @@
                 try:
                     for line in file(self.options.part):
                         elements = line.strip().split(' ')
-                        tmpfile = util.tmpfile(keep=False)
+                        tmpfile = util.tmp_filename(
+                            tmp_root=self.options.tmp_root)
                         if elements[0] == 'root':
                             hypervisor.add_filesystem(elements[1], default_filesystem, filename=tmpfile, mntpnt='/')
                         elif elements[0] == 'swap':
@@ -256,10 +284,12 @@
 
                 except IOError, (errno, strerror):
                     hypervisor.optparser.error("%s parsing --part option: %s" % (errno, strerror))
-    
+
     def do_disk(self, hypervisor, curdisk, size):
         default_filesystem = hypervisor.distro.preferred_filesystem()
-        disk = hypervisor.add_disk(util.tmpfile(keep=False), size+1)
+        disk = hypervisor.add_disk(
+            util.tmp_filename(tmp_root=self.options.tmp_root),
+            size+1)
         logging.debug("do_disk - size: %d" % size)
         offset = 0
         for pair in curdisk:

=== modified file 'VMBuilder/util.py'
--- VMBuilder/util.py	2010-02-25 23:41:18 +0000
+++ VMBuilder/util.py	2010-06-10 17:56:24 +0000
@@ -168,18 +168,36 @@
     logging.debug('No such method')
     return
 
-def tmpfile(suffix='', keep=True):
-    (fd, filename) = tempfile.mkstemp(suffix=suffix)
-    os.close(fd)
-    if not keep:
-        os.unlink(filename)
-    return filename
-
-def tmpdir(suffix='', keep=True):
-    dir = tempfile.mkdtemp(suffix=suffix)
-    if not keep:
-        os.rmdir(dir)
-    return dir
+def tmp_filename(suffix='', tmp_root=None):
+    # There is a risk in using tempfile.mktemp(): it's not recommended
+    # to run vmbuilder on machines with untrusted users.
+    return tempfile.mktemp(suffix=suffix, dir=tmp_root)
+
+def tmpdir(suffix='', tmp_root=None):
+    return tempfile.mkdtemp(suffix=suffix, dir=tmp_root)
+
+def set_up_tmpfs(tmp_root=None, size=1024):
+    """Sets up a tmpfs storage under `tmp_root` with the size of `size` MB.
+
+    `tmp_root` defaults to tempfile.gettempdir().
+    """
+    mount_point = tmpdir('tmpfs', tmp_root)
+    mount_cmd = ["mount", "-t", "tmpfs",
+                 "-o", "size=%dM,mode=0770" % int(size),
+                 "tmpfs", mount_point ]
+    logging.info('Mounting tmpfs under %s' % mount_point)
+    logging.debug('Executing: %s' % mount_cmd)
+    run_cmd(*mount_cmd)
+
+    return mount_point
+
+def clean_up_tmpfs(mount_point):
+    """Unmounts a tmpfs storage under `mount_point`."""
+    umount_cmd = ["umount", "-t", "tmpfs", mount_point ]
+    logging.info('Unmounting tmpfs from %s' % mount_point)
+    logging.debug('Executing: %s' % umount_cmd)
+    run_cmd(*umount_cmd)
+
 
 def get_conf_value(context, confparser, key):
     confvalue = None