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?

Ok well spotted. The assumption is that they are using a regular UEC image. LXC container support has already been added in the latest imgaes.

>
> > + 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..
>

It wont, its getting the information associated with the associated image.

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

Deleting it when the image is shutdown.

>
> 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.
>

Sure I can do that.

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