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

Ok, Chuck just pointed out to me that he stopped using kpartx altogether, which I completely missed in my last review, so here's another review where I stop being an idiot.

> === modified file 'nova/virt/disk.py'
> --- nova/virt/disk.py 2011-03-12 23:53:10 +0000
> +++ nova/virt/disk.py 2011-03-15 10:46:53 +0000
> @@ -115,6 +115,34 @@
> _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', mapped_device, container_dir)

What's mapped_device? I'm not really sure what the assumptions here are. In what format are you expecting these LXC images to be distributed?

> + if err:

The comment from my previous review still applies here.

> + 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"""
> + try:
> + container_dir = '%s/rootfs' % target
> + utils.execute('sudo', 'umount', container_dir)
> + finally:
> + image = os.path.join(FLAGS.instances_path, instance['name'], '' + 'disk')
> + out, err = utils.execute('sudo', 'losetup', '--find', '--show', image)

I still think this will create another loopback device..

> + device = out.strip()
> + if err:
> + raise execption.Error(_('Could not find loopback image: %s')
> + %err)
> + utils.execute('sudo', 'losetup', '--detach', device)

...and then remove it again?

With regard to unit tests, it would be nice if they'd check:
 * if os/type was set to exe,
 * if os/init was set to /sbin/init,
 * and that that filesystem was set correctly.

After all, the number of conditionals in that template is making it a bit difficult to verify by just looking at it.