Code review comment for lp:~zulcss/nova/nova-lxc

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