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

> === modified file 'nova/virt/disk.py'
> --- nova/virt/disk.py 2011-03-17 15:29:04 +0000
> +++ nova/virt/disk.py 2011-03-24 10:26:05 +0000
> @@ -115,6 +115,37 @@
> _unlink_device(device, nbd)
>
>
> +def setup_container(image, container_dir=None, partition=None):

You don't use the partition kwarg, so don't pass it. It's confusing.

> + """Setup the LXC container
> +
> + It will mount the loopback image to the container directory in order
> + to create the root filesystem for the container
> + """
> + nbd = False
> + 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):
> + """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 utils.popen('sudo losetup -a').readlines():

There's no such things as utils.popen.

> + if instance['name'] in loop:
> + device = loop.split(loop, ':')
> + utils.execute('sudo', 'losetup', '--detach', device)
> +
> +
> def _link_device(image, nbd):
> """Link image to device using loopback or nbd"""
> if nbd:
>
> === modified file 'nova/virt/libvirt.xml.template'
> --- nova/virt/libvirt.xml.template 2011-03-23 19:46:24 +0000
> +++ nova/virt/libvirt.xml.template 2011-03-24 10:26:05 +0000
> @@ -2,6 +2,12 @@
> <name>${name}</name>
> <memory>${memory_kb}</memory>
> <os>
> +#if $type == 'lxc'
> + #set $disk_prefix = ''
> + #set $disk_bus = ''
> + <type>exe</type>
> + <init>/sbin/init</init>
> +#else
> #if $type == 'uml'
> #set $disk_prefix = 'ubd'
> #set $disk_bus = 'uml'
> @@ -37,6 +43,7 @@
> <boot dev="hd" />
> #end if
> #end if
> + #end if

Your indentations don't line up.

> #end if
> </os>
> <features>
> @@ -44,6 +51,12 @@
> </features>
> <vcpu>${vcpus}</vcpu>
> <devices>
> +#if $type == 'lxc'
> + <filesystem type='mount'>
> + <source dir='${basepath}/rootfs'/>
> + <target dir='/'/>
> + </filesystem>
> +#else

You're still missing a unit test for this. What's the difficulty?

> #if $getVar('rescue', False)
> <disk type='file'>
> <driver type='${driver_type}'/>
> @@ -68,6 +81,7 @@
> <target dev='${disk_prefix}b' bus='${disk_bus}'/>
> </disk>
> #end if
> + #end if

Again, odd indentation. It's hard enough to read as it is. Please make sure that #if's line up with their corresponding "#end if"s.

> === modified file 'nova/virt/libvirt_conn.py'
> --- nova/virt/libvirt_conn.py 2011-03-24 10:15:52 +0000
> +++ nova/virt/libvirt_conn.py 2011-03-24 10:26:05 +0000
> @@ -20,7 +20,7 @@
> """
> A connection to a hypervisor through libvirt.
>
> -Supports KVM, QEMU, UML, and XEN.
> +Supports KVM, LXC, QEMU, UML, and XEN.
>
> **Related Flags**
>
> @@ -85,7 +85,7 @@
> flags.DEFINE_string('libvirt_type',
> 'kvm',
> 'Libvirt domain type (valid options are: '
> - 'kvm, qemu, uml, xen)')
> + 'kvm, lxc, qemu, uml, xen)')
> flags.DEFINE_string('libvirt_uri',
> '',
> 'Override the default libvirt URI (which is dependent'
> @@ -218,6 +218,8 @@
> uri = FLAGS.libvirt_uri or 'uml:///system'
> elif FLAGS.libvirt_type == 'xen':
> uri = FLAGS.libvirt_uri or 'xen:///'
> + elif FLAGS.libvirt_type == 'lxc':
> + uri = FLAGS.libvirt_uri or 'lxc:///'
> else:
> uri = FLAGS.libvirt_uri or 'qemu:///system'
> return uri
> @@ -296,6 +298,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)
> if os.path.exists(target):
> shutil.rmtree(target)
>
> @@ -518,6 +522,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"))
> else:
> fpath = console_log
>
> @@ -624,6 +631,10 @@
> f.write(libvirt_xml)
> f.close()
>
> + if FLAGS.libvirt_type == 'lxc':
> + container_dir = '%s/rootfs' % basepath(suffix='')
> + utils.execute('mkdir', '-p', container_dir)
> +
> # NOTE(vish): No need add the suffix to console.log
> os.close(os.open(basepath('console.log', ''),
> os.O_CREAT | os.O_WRONLY, 0660))
> @@ -683,6 +694,9 @@
> if not inst['kernel_id']:
> target_partition = "1"
>
> + if FLAGS.libvirt_type == 'lxc':
> + target_partition = None
> +
> key = str(inst['key_data'])
> net = None
> network_ref = db.network_get_by_instance(context.get_admin_context(),
> @@ -720,6 +734,11 @@
> 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)
> except Exception as e:
> # This could be a windows image, or a vmdk format disk
> LOG.warn(_('instance %(inst_name)s: ignoring error injecting'