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

2011/3/14 Chuck Short <email address hidden>:
> Soren,
>
> Please see:
>
> http://bazaar.launchpad.net/~zulcss/nova/nova-lxc/revision/758

I did, but I still don't see it. Ok, from the top:

=== modified file 'nova/virt/disk.py'
--- nova/virt/disk.py 2011-03-12 23:53:10 +0000
+++ nova/virt/disk.py 2011-03-14 20:56:29 +0000
@@ -115,6 +115,39 @@
         _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)

Ok, so here you attach the disk image through nbd or losetup.

+ try:
+ if not partition is None:
+ # create partition
+ utils.execute('sudo kpartx -a %s' % device)

This needs to be changed to utils.execute('sudo', 'kparts', '-a', etc)..

Also, why is this needed? nbd devices should benefit from the kernel's
built in partition detection. If it's for non-nbd devices, make it
conditional.

+ mapped_device = '/dev/mapper/%p%s' % (device.split('/')[-1],
+ partition)
+ else:
+ mapped_device = device
+
+ utils.execute('sudo mount %s %s' %(mapped_device, container_dir))

This too needs to be changed to pass individual argv elements separately.

+
+ except Exception as e:
+ LOG.warn(_('Unable to mount container'))
+ if not partition is None:
+ # remove partitions
+ utils.execute('sudo kpartx -s %s' % device)

What does "kpartx -s" do?

+ _unlink_device(device, nbd)
+
+def destroy_container(target, instance, nbd=False):
+ """Destroy the container once it terminates"""
+ try:
+ utils.execute('sudo umount %s/rootfs' % target)

Here you unmount the rootfs. What about the mapped partitions from
kparts and the nbd device. Where do they get cleaned up?

+ image = os.path.join(FLAGS.instances_path, instance['name'],
'' + 'disk')

What's this for?

+ except Exception as e:

This is scary. Can you make this more specific (and you can leave off
the "as e" bit if you're not using the exception for anything.

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/