> === modified file 'Authors'
> --- Authors	2011-03-15 18:19:47 +0000
> +++ Authors	2011-03-17 12:08:41 +0000
> @@ -11,6 +11,7 @@
>  Chmouel Boudjnah <email address hidden>
>  Chris Behrens <email address hidden>
>  Christian Berendt <email address hidden>
> +Chuck Short <email address hidden>

If you want a different e-mail here, you can look at the .mailmap file.

>  Cory Wright <email address hidden>
>  Dan Prince <email address hidden>
>  David Pravec <email address hidden>
> 
> === modified file 'nova/tests/test_virt.py'
> --- nova/tests/test_virt.py	2011-03-14 17:59:41 +0000
> +++ nova/tests/test_virt.py	2011-03-17 12:08:41 +0000
[...]
> @@ -302,8 +342,9 @@
>              (lambda t: t.find('./devices/serial/source').get(
>                  'path').split('/')[1], 'console.log'),
>              (lambda t: t.find('./memory').text, '2097152')]
> +
>          if rescue:
> -            common_checks += [
> +            common_checks = [

Why? That will effectively disable a bunch of checks.

>                  (lambda t: t.findall('./devices/disk/source')[0].get(
>                      'file').split('/')[1], 'disk.rescue'),
>                  (lambda t: t.findall('./devices/disk/source')[1].get(
> @@ -326,14 +367,14 @@
>              xml = conn.to_xml(instance_ref, rescue)
>              tree = xml_to_tree(xml)
>              for i, (check, expected_result) in enumerate(checks):
> -                self.assertEqual(check(tree),
> -                                 expected_result,
> -                                 '%s failed check %d' % (xml, i))
> +                        self.assertEqual(check(tree),
> +                                        expected_result,
> +                                        '%s failed check %d' % (xml, i))

What's going on here?

>  
>              for i, (check, expected_result) in enumerate(common_checks):
> -                self.assertEqual(check(tree),
> -                                 expected_result,
> -                                 '%s failed common check %d' % (xml, i))
> +                   self.assertEqual(check(tree),
> +                                    expected_result,
> +                                    '%s failed common check %d' % (xml, i))

...and here?

> === modified file 'nova/virt/disk.py'
> --- nova/virt/disk.py	2011-03-14 17:59:41 +0000
> +++ nova/virt/disk.py	2011-03-17 12:08:41 +0000
> @@ -23,6 +23,7 @@
>  """
>  
>  import os
> +import string
>  import tempfile
>  import time
>  
> @@ -115,6 +116,36 @@
>          _unlink_device(device, nbd)
>  
>  
> +def setup_container(image, container_dir=None, partition=None, nbd=False):
> +    """Setup the LXC container
> +
> +    It will mount the loopback image to the container directory in order
> +    to create the root filesystem for the container
> +    """
> +    device = _link_device(image, nbd)
> +    err = utils.execute('sudo', 'mount', device, container_dir)
> +    if err:
> +        raise exception.Error(_('Failed to mount filesystem: %s')
> +                             % err)
> +        _unlink_device(device, nbd)
> +
> +
> +def destroy_container(target, instance, nbd=False):
> +    """Destroy the container once it terminates
> +    
> +    It will umount the container that is mounted, try to find the loopback
> +    device associated with the container and delete it.
> +    """
> +    try:
> +        container_dir = '%s/rootfs' % target
> +        utils.execute('sudo', 'umount', container_dir)
> +    finally:
> +        for loop in os.popen('sudo losetup -a').readlines():

Why use os.popen instead of utils.execute?

> +            if instance['name'] in loop:
> +                device = string.split(loop, ':')

Don't use string.split this way. You can just do: loop.split(':')

When you make that change, you can also get rid of "import string".

> === modified file 'nova/virt/libvirt_conn.py'
> --- nova/virt/libvirt_conn.py	2011-03-16 19:26:41 +0000
> +++ nova/virt/libvirt_conn.py	2011-03-17 12:08:41 +0000
> @@ -272,6 +274,8 @@
>          instance_name = instance['name']
>          LOG.info(_('instance %(instance_name)s: deleting instance files'
>                  ' %(target)s') % locals())
> +        if FLAGS.libvirt_type == 'lxc':
> +            disk.destroy_container(target, instance, nbd=FLAGS.use_cow_images)
>          if os.path.exists(target):
>              shutil.rmtree(target)
>  

Since you've removed support for cow images and don't use nbd for LXC,
it's confusing that you're still passing an argument to enable/disable this.

> @@ -486,6 +490,9 @@
>                                           instance['name'])
>              data = self._flush_xen_console(virsh_output)
>              fpath = self._append_to_file(data, console_log)
> +        elif FLAGS.libvirt_type == 'lxc':
> +            # LXC is also special
> +            LOG.info(_("Unable to read LXC console"))

Wow, really? LXC doesn't have any notion of a console? Can you only speak to it over ssh or something?

> @@ -652,6 +663,9 @@
>          if not inst['kernel_id']:
>              target_partition = "1"
>  
> +        if FLAGS.libvirt_type == 'lxc':
> +            target_partition = None
> +

Here, you hard code target_partition..

>          key = str(inst['key_data'])
>          net = None
>          network_ref = db.network_get_by_instance(context.get_admin_context(),
> @@ -682,6 +696,12 @@
>                  disk.inject_data(basepath('disk'), key, net,
>                                   partition=target_partition,
>                                   nbd=FLAGS.use_cow_images)
> +
> +                if FLAGS.libvirt_type == 'lxc':
> +                    disk.setup_container(basepath('disk'),
> +                                        container_dir=container_dir,
> +                                        partition=target_partition,
> +                                        nbd=FLAGS.use_cow_images)

...and here you pass it. AFAICT, setup_container doesn't even use it anymore?


Also, I don't see the tests for the filesystem stuff in the libvirt xml?