Merge lp:~zulcss/nova/nova-lxc into lp:~hudson-openstack/nova/trunk

[Chuck Short]

This branch adds support for linux containers (LXC) to nova. It uses the libvirt LXC driver to start and stop the instance.

[Vish Ishaya (vishvananda)]

While I'm very excited about using lxc with nova, I am unable to get this to work. The instance reports being launched correctly but doesn't seem to actually succeed
virsh --connect lxc:/// list shows nothing and I get the following error in
/var/log/libvirt/lxc/instance-00000001.log
17:09:05.465: error : lxcFdForward:231 : read of fd 7 failed: Input/output error
Can we get some sort of instructions as to the setup necessary to make this work. Does it require a special image? Does the image have to match the host os? Do we need to do special setup to get it to work? apt-get install lxc? anything else?

FWIW I am testing using the ttylinux (ami-tty). Maverick is my host os.

review: Needs Information

[Vish Ishaya (vishvananda)]

Ok, I think I figured this out mostly. I created an ubuntu image using lxc-ubuntu and it seems to work properly. The busybox image launches properly as well but doesn't seem to be running sshd. I think having a ttylinux/busybox version that runs sshd and cloud-init would be very useful for testing.

[Jay Pipes (jaypipes)]

Hi Chuck!

149 + utils.execute('mkdir -p %s' % container_dir)

Does the above need a sudo?

Also, I note that there are no test cases included for this new functionality. Could we add some?

Cheers!
jay

review: Needs Information

[Chuck Short (zulcss)]

> Hi Chuck!
>
> 149 + utils.execute('mkdir -p %s' % container_dir)
>
> Does the above need a sudo?
>
> Also, I note that there are no test cases included for this new functionality.
> Could we add some?
>
> Cheers!
> jay

Hi Jay,

It doesnt need sudo because its running as the nova user. Also the tests were added in revision 753.

chuck

[Chuck Short (zulcss)]

> While I'm very excited about using lxc with nova, I am unable to get this to
> work. The instance reports being launched correctly but doesn't seem to
> actually succeed
> virsh --connect lxc:/// list shows nothing and I get the following error in
> /var/log/libvirt/lxc/instance-00000001.log
> 17:09:05.465: error : lxcFdForward:231 : read of fd 7 failed: Input/output
> error
> Can we get some sort of instructions as to the setup necessary to make this
> work. Does it require a special image? Does the image have to match the host
> os? Do we need to do special setup to get it to work? apt-get install lxc?
> anything else?
>
> FWIW I am testing using the ttylinux (ami-tty). Maverick is my host os.

Right Ive added some documentation. It can be found at:

http://wiki.openstack.org/LXC

You will also need a newer version of libvirt (preferably the one from natty).

chuck

[Vish Ishaya (vishvananda)]

Ok, we definitely need to add the new libvirt to our ppa to support older installs.

I'm still working on getting this running properly. I'll let you know if i run into any issues.

Vish

On Feb 28, 2011, at 10:59 AM, Chuck Short wrote:

>> While I'm very excited about using lxc with nova, I am unable to get this to
>> work. The instance reports being launched correctly but doesn't seem to
>> actually succeed
>> virsh --connect lxc:/// list shows nothing and I get the following error in
>> /var/log/libvirt/lxc/instance-00000001.log
>> 17:09:05.465: error : lxcFdForward:231 : read of fd 7 failed: Input/output
>> error
>> Can we get some sort of instructions as to the setup necessary to make this
>> work. Does it require a special image? Does the image have to match the host
>> os? Do we need to do special setup to get it to work? apt-get install lxc?
>> anything else?
>>
>> FWIW I am testing using the ttylinux (ami-tty). Maverick is my host os.
>
> Right Ive added some documentation. It can be found at:
>
> http://wiki.openstack.org/LXC
>
> You will also need a newer version of libvirt (preferably the one from natty).
>
> chuck
> --
> https://code.launchpad.net/~zulcss/nova/nova-lxc/+merge/51469
> You are reviewing the proposed merge of lp:~zulcss/nova/nova-lxc into lp:nova.

[Soren Hansen (soren)]

You don't seem to ever free the loopback you set up in setup_container?

review: Needs Information

[Chuck Short (zulcss)]

http://bazaar.launchpad.net/~zulcss/nova/nova-lxc/revision/752

[Soren Hansen (soren)]

2011/3/7 Chuck Short <email address hidden>:
> http://bazaar.launchpad.net/~zulcss/nova/nova-lxc/revision/752

AFAICT, that will still leak loopback devices (and corresponding
mapped partitions)?

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

[Chuck Short (zulcss)]

> 2011/3/7 Chuck Short <email address hidden>:
> > http://bazaar.launchpad.net/~zulcss/nova/nova-lxc/revision/752
>
> AFAICT, that will still leak loopback devices (and corresponding
> mapped partitions)?
>
> --
> Soren Hansen        | http://linux2go.dk/
> Ubuntu Developer    | http://www.ubuntu.com/
> OpenStack Developer | http://www.openstack.org/

Ok I think I cleared this up in the last commit.

chuck

[Nachi Ueno (nati-ueno)]

LXC support on Nova is pretty cool!
Would you please add procedure in http://wiki.openstack.org/LXC to nova.sh script ?

[Jay Pipes (jaypipes)]

lgtm.

review: Approve

[Soren Hansen (soren)]

> > 2011/3/7 Chuck Short <email address hidden>:
> > > http://bazaar.launchpad.net/~zulcss/nova/nova-lxc/revision/752
> > AFAICT, that will still leak loopback devices (and corresponding
> > mapped partitions)?
> Ok I think I cleared this up in the last commit.

Sorry, I don't see it. Can you explain how the partition devices and loopback devices get cleaned up? It's not obvious to me, I'm afraid.

review: Needs Information

[Chuck Short (zulcss)]

Soren,

Please see:

http://bazaar.launchpad.net/~zulcss/nova/nova-lxc/revision/758

chuck

[Soren Hansen (soren)]

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/

[Soren Hansen (soren)]

> === 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)
> + if err:

This will always evaluate as True. utils.execute returns a 2-tuple.

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

You're still not clearing out the mapped partitions that you create with kpartx. Instead you seem to be creating an additional loopback device or is that not what losetup --find does?

review: Needs Fixing

[Soren Hansen (soren)]

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.

review: Needs Fixing

[Chuck Short (zulcss)]

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

[Soren Hansen (soren)]

2011/3/15 Chuck Short <email address hidden>:
>> 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.

So only raw disk images are supported? Ok.

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

[Soren Hansen (soren)]

My review seems to have gotten cut short. :( /me retries

[Soren Hansen (soren)]

These are the missing bits:

>> I still think this will create another loopback device..
> It wont, its getting the information associated with the associated image.

What am I doing wrong/differently here:
[2011-03-15 13:45:03] soren@lenny:/tmp/loopbacktest$ dd if=/dev/zero
of=testimg.img bs=512 count=1k
1024+0 blokke ind
1024+0 blokke ud
524288 byte (524 kB) kopieret, 0,0075592 s, 69,4 MB/s
[2011-03-15 13:45:15] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
[sudo] password for soren:
/dev/loop0
[2011-03-15 13:45:38] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
/dev/loop1
[2011-03-15 13:45:38] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
/dev/loop2
[2011-03-15 13:45:39] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
/dev/loop3
[2011-03-15 13:45:39] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
/dev/loop4
[2011-03-15 13:45:39] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
/dev/loop5
[2011-03-15 13:45:40] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
/dev/loop6
[2011-03-15 13:45:40] soren@lenny:/tmp/loopbacktest$ sudo losetup
--find --show $(pwd)/testimg.img
/dev/loop7

Each time I get a new loopback device.

[Soren Hansen (soren)]

In my defense, it was must lovelier formatted when I sent it the first time. :-/

[Chuck Short (zulcss)]

> In my defense, it was must lovelier formatted when I sent it the first time.
> :-/

Hah..

I think I addressed your comments, it should be ready to go now.

chuck

[Soren Hansen (soren)]

======================================================================
ERROR: Failure: SyntaxError (can't assign to function call (test_virt.py, line 262))
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/nose/loader.py", line 382, in loadTestsFromName
    addr.filename, addr.module)
  File "/usr/lib/pymodules/python2.7/nose/importer.py", line 39, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/usr/lib/pymodules/python2.7/nose/importer.py", line 86, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/home/soren/src/openstack/nova/nova/nova/tests/test_virt.py", line 262
    for i (check, expected_result) in enumerate(check):
SyntaxError: can't assign to function call

======================================================================
FAIL: test_authors_up_to_date (nova.tests.test_misc.ProjectTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/soren/src/openstack/nova/nova/nova/tests/test_misc.py", line 59, in test_authors_up_to_date
    '%r not listed in Authors' % missing)
AssertionError: set([u'<email address hidden>']) not listed in Authors

----------------------------------------------------------------------

[Chuck Short (zulcss)]

> ======================================================================
> ERROR: Failure: SyntaxError (can't assign to function call (test_virt.py, line
> 262))
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/lib/pymodules/python2.7/nose/loader.py", line 382, in
> loadTestsFromName
> addr.filename, addr.module)
> File "/usr/lib/pymodules/python2.7/nose/importer.py", line 39, in
> importFromPath
> return self.importFromDir(dir_path, fqname)
> File "/usr/lib/pymodules/python2.7/nose/importer.py", line 86, in
> importFromDir
> mod = load_module(part_fqname, fh, filename, desc)
> File "/home/soren/src/openstack/nova/nova/nova/tests/test_virt.py", line 262
> for i (check, expected_result) in enumerate(check):
> SyntaxError: can't assign to function call
>
> ======================================================================
> FAIL: test_authors_up_to_date (nova.tests.test_misc.ProjectTestCase)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/soren/src/openstack/nova/nova/nova/tests/test_misc.py", line 59,
> in test_authors_up_to_date
> '%r not listed in Authors' % missing)
> AssertionError: set([u'<email address hidden>']) not listed in Authors
>
> ----------------------------------------------------------------------

Ok thats fixed now...test suite runs cleanly now.

[Soren Hansen (soren)]

nova/tests/test_virt.py:375:20: E111 indentation is not a multiple of four
                   self.assertEqual(check(tree),
                   ^
    Use 4 spaces per indentation level.

    For really old code that you don't want to mess up, you can continue to
    use 8-space tabs.

    Okay: a = 1
    Okay: if a == 0:\n a = 1
    E111: a = 1

    Okay: for item in items:\n pass
    E112: for item in items:\npass

    Okay: a = 1\nb = 2
    E113: a = 1\n b = 2
nova/virt/disk.py:135:1: W293 blank line contains whitespace

^
    JCR: Trailing whitespace is superfluous.
    FBM: Except when it occurs as part of a blank line (i.e. the line is
         nothing but whitespace). According to Python docs[1] a line with only
         whitespace is considered a blank line, and is to be ignored. However,
         matching a blank line to its indentation level avoids mistakenly
         terminating a multi-line statement (e.g. class declaration) when
         pasting code into the standard Python interpreter.

         [1] http://docs.python.org/reference/lexical_analysis.html#blank-lines

    The warning returned varies on whether the line itself is blank, for easier
    filtering for those who want to indent their blank lines.

    Okay: spam(1)
    W291: spam(1)\s
    W293: class Foo(object):\n \n bang = 12

[Chuck Short (zulcss)]

> nova/tests/test_virt.py:375:20: E111 indentation is not a multiple of four
> self.assertEqual(check(tree),
> ^
> Use 4 spaces per indentation level.
>
> For really old code that you don't want to mess up, you can continue to
> use 8-space tabs.
>
> Okay: a = 1
> Okay: if a == 0:\n a = 1
> E111: a = 1
>
> Okay: for item in items:\n pass
> E112: for item in items:\npass
>
> Okay: a = 1\nb = 2
> E113: a = 1\n b = 2
> nova/virt/disk.py:135:1: W293 blank line contains whitespace
>
> ^
> JCR: Trailing whitespace is superfluous.
> FBM: Except when it occurs as part of a blank line (i.e. the line is
> nothing but whitespace). According to Python docs[1] a line with only
> whitespace is considered a blank line, and is to be ignored. However,
> matching a blank line to its indentation level avoids mistakenly
> terminating a multi-line statement (e.g. class declaration) when
> pasting code into the standard Python interpreter.
>
> [1] http://docs.python.org/reference/lexical_analysis.html#blank-
> lines
>
> The warning returned varies on whether the line itself is blank, for
> easier
> filtering for those who want to indent their blank lines.
>
> Okay: spam(1)
> W291: spam(1)\s
> W293: class Foo(object):\n \n bang = 12

And thats fixed.

[Soren Hansen (soren)]

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

review: Needs Fixing

[Soren Hansen (soren)]

> > nova/tests/test_virt.py:375:20: E111 indentation is not a multiple of four
[...]
> > nova/virt/disk.py:135:1: W293 blank line contains whitespace
> And thats fixed.

Um.. No. Maybe you forgot to commit/push?

[Chuck Short (zulcss)]

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

This was a left over when I was trying to add tests for lxc.
>
> > (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?

ditto

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

done

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

[Rick Clark (dendrobates)]

I've tested this and it works for me. I am approving this, but I want to make sure that Soren is satisfied that all his concerns were addressed before merging it.

review: Approve

[Soren Hansen (soren)]

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

review: Needs Fixing

[Soren Hansen (soren)]

> === modified file 'nova/virt/disk.py'
> --- nova/virt/disk.py 2011-03-14 17:59:41 +0000
> +++ nova/virt/disk.py 2011-03-24 22:32:44 +0000
> @@ -115,6 +115,38 @@
> _unlink_device(device, nbd)
>
>
> +def setup_container(image, container_dir=None, partition=None):

Still accepting an unused partition argument.

> + """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:

As I've pointed out before, utils.execute returns a 2-tuple. 2-tuples evaluate to True, so as far as I can see, this will always evaluate to True?

> + 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:
> + out, err = utils('sudo', 'losetup', '-a')

There's no function named utils.

> + for loop in out.splitlines():
> + 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-02 01:12:47 +0000
> +++ nova/virt/libvirt.xml.template 2011-03-24 22:32:44 +0000

Ok, if you take what you have there now and apply this patch:

    http://paste.ubuntu.com/585085/plain/

...then it's fine. Indentation-wise, at least.

> === modified file 'nova/virt/libvirt_conn.py'
> --- nova/virt/libvirt_conn.py 2011-03-24 17:53:54 +0000
> +++ nova/virt/libvirt_conn.py 2011-03-24 22:32:44 +0000
> @@ -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)

...and still passing the unused partition argument.

review: Needs Fixing

[Soren Hansen (soren)]

This is a very cool feature, by the way. I'm really looking forward to getting it in. We're close :)

[Chuck Short (zulcss)]

> > === modified file 'nova/virt/disk.py'
> > --- nova/virt/disk.py 2011-03-14 17:59:41 +0000
> > +++ nova/virt/disk.py 2011-03-24 22:32:44 +0000
> > @@ -115,6 +115,38 @@
> > _unlink_device(device, nbd)
> >
> >
> > +def setup_container(image, container_dir=None, partition=None):
>
> Still accepting an unused partition argument.
>

Fixed

> > + """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:
>
> As I've pointed out before, utils.execute returns a 2-tuple. 2-tuples evaluate
> to True, so as far as I can see, this will always evaluate to True?

There is the case where the nbd module is not loaded when the device is created so probably the the check be something like: (pseudocode)

if device:
  raise exception

>
> > + 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:
> > + out, err = utils('sudo', 'losetup', '-a')
>
> There's no function named utils.
>

fixed

> > + for loop in out.splitlines():
> > + 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-02 01:12:47 +0000
> > +++ nova/virt/libvirt.xml.template 2011-03-24 22:32:44 +0000
>
> Ok, if you take what you have there now and apply this patch:
>
> http://paste.ubuntu.com/585085/plain/
>
> ...then it's fine. Indentation-wise, at least.
>
> > === modified file 'nova/virt/libvirt_conn.py'
> > --- nova/virt/libvirt_conn.py 2011-03-24 17:53:54 +0000
> > +++ nova/virt/libvirt_conn.py 2011-03-24 22:32:44 +0000
> > @@ -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)
>
> ...and still passing the unused partition argument.

One or more two things, I havent added the test for the filesystem because i havent f...

[Soren Hansen (soren)]

2011/3/25 Chuck Short <email address hidden>:
>> > +    """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:
>>
>> As I've pointed out before, utils.execute returns a 2-tuple. 2-tuples evaluate
>> to True, so as far as I can see, this will always evaluate to True?
> There is the case where the nbd module is not loaded when the device is created so probably the the check be something like: (pseudocode)
>
> if device:
>  raise exception

You've lost me entirely. My point is this: utils.execute returns a
2-tuple (or raises an exception). The 2-tuple consists of what was
output on the process' stdout and stderr, respectively. You're storing
this 2-tuple in a single variable, so "if err" will, as far as I can
see, always evaluate to True? If that is so, aren't you always
unmounting your container immediately after mounting it?

> One or more two things, I havent added the test for the filesystem because i havent figured out how to access the chunk in the xml.

There are a number of examples of poking around in the XML in the
libvirt tests already. There are even checks that verify that the disk
paths are set correctly based on whether it's a rescue system or not.
Can you share what you've tried and we can try to work it out together?

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

[Thierry Carrez (ttx)]

Interesting feature, self-contained, almost there: FFE granted for late merging by Tuesday EOD

review: Approve (ffe)

[termie (termie)]

- you've got extra whitespaces at lines: 33, 38
- 41: you should be using self.flags(libvirt_type='lxc') to override flags so that they are automatically reset at the end of the test
- your test seems to rely on libvirt being available which is not always the case, perhaps you should skip the test if libvirt is not available? otherwise it doesn't seem like it is testing anything.
- at line 51 you should move line 52 up to that line and indent line 53 to to match it
- 71: please add punctuation to your docstring
- 81: missing a space on your indent
- in the new xml template you appear to have removed a feature related to multiple nics, what was the reasoning behind that?

other than that stuff, looks pretty cool :)

review: Needs Fixing

[Vish Ishaya (vishvananda)]

termie mentioned this, but it appears that the following is all due to a bad merge:

         <interface type='bridge'>
255 - <source bridge='${nic.bridge_name}'/>
256 - <mac address='${nic.mac_address}'/>
257 + <source bridge='${bridge_name}'/>
258 + <mac address='${mac_address}'/>
259 <!-- <model type='virtio'/> CANT RUN virtio network right now -->
260 - <filterref filter="nova-instance-${name}-${nic.id}">
261 - <parameter name="IP" value="${nic.ip_address}" />
262 - <parameter name="DHCPSERVER" value="${nic.dhcp_server}" />
263 -#if $getVar('nic.extra_params', False)
264 - ${nic.extra_params}
265 + <filterref filter="nova-instance-${name}">
266 + <parameter name="IP" value="${ip_address}" />
267 + <parameter name="DHCPSERVER" value="${dhcp_server}" />
268 +#if $getVar('extra_params', False)
269 + ${extra_params}
270 #end if
271 -#if $getVar('nic.gateway_v6', False)
272 - <parameter name="RASERVER" value="${nic.gateway_v6}" />
273 +#if $getVar('gateway_v6', False)
274 + <parameter name="RASERVER" value="${gateway_v6}" />
275 #end if

review: Needs Fixing

[lp:~zulcss/nova/nova-lxc]

804. By Chuck Short on 2011-03-28

Merge trunk

805. By Chuck Short on 2011-03-28

Fix libvirt merge mistake

806. By Chuck Short on 2011-03-28

Add more unit tests for lxc

807. By Chuck Short on 2011-03-28

More pep8 corrections

808. By Chuck Short on 2011-03-28

Dont make the test fail

809. By Chuck Short on 2011-03-28

use self.flags in virt test

[Chuck Short (zulcss)]

> 2011/3/25 Chuck Short <email address hidden>:
> >> > +    """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:
> >>
> >> As I've pointed out before, utils.execute returns a 2-tuple. 2-tuples
> evaluate
> >> to True, so as far as I can see, this will always evaluate to True?
> > There is the case where the nbd module is not loaded when the device is
> created so probably the the check be something like: (pseudocode)
> >
> > if device:
> >  raise exception
>
> You've lost me entirely. My point is this: utils.execute returns a
> 2-tuple (or raises an exception). The 2-tuple consists of what was
> output on the process' stdout and stderr, respectively. You're storing
> this 2-tuple in a single variable, so "if err" will, as far as I can
> see, always evaluate to True? If that is so, aren't you always
> unmounting your container immediately after mounting it?

I see what you are saying now I have corrected this now.

>
> > One or more two things, I havent added the test for the filesystem because i
> havent figured out how to access the chunk in the xml.
>
> There are a number of examples of poking around in the XML in the
> libvirt tests already. There are even checks that verify that the disk
> paths are set correctly based on whether it's a rescue system or not.
> Can you share what you've tried and we can try to work it out together?

I didnt think of looking at the libvirt tests. I believe I have added the tests that you have wanted.

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

[Chuck Short (zulcss)]

> termie mentioned this, but it appears that the following is all due to a bad
> merge:
>
> <interface type='bridge'>
> 255 - <source bridge='${nic.bridge_name}'/>
> 256 - <mac address='${nic.mac_address}'/>
> 257 + <source bridge='${bridge_name}'/>
> 258 + <mac address='${mac_address}'/>
> 259 <!-- <model type='virtio'/> CANT RUN virtio network
> right now -->
> 260 - <filterref filter="nova-instance-${name}-${nic.id}">
> 261 - <parameter name="IP" value="${nic.ip_address}" />
> 262 - <parameter name="DHCPSERVER"
> value="${nic.dhcp_server}" />
> 263 -#if $getVar('nic.extra_params', False)
> 264 - ${nic.extra_params}
> 265 + <filterref filter="nova-instance-${name}">
> 266 + <parameter name="IP" value="${ip_address}" />
> 267 + <parameter name="DHCPSERVER" value="${dhcp_server}"
> />
> 268 +#if $getVar('extra_params', False)
> 269 + ${extra_params}
> 270 #end if
> 271 -#if $getVar('nic.gateway_v6', False)
> 272 - <parameter name="RASERVER" value="${nic.gateway_v6}"
> />
> 273 +#if $getVar('gateway_v6', False)
> 274 + <parameter name="RASERVER" value="${gateway_v6}" />
> 275 #end if

It was a bad merge, I have corrected it.

[lp:~zulcss/nova/nova-lxc]

810. By Chuck Short on 2011-03-28

Fix up libvirt.xml.template

811. By Chuck Short on 2011-03-29

Pass along the nbd flags although we dont support it just yet

812. By Chuck Short on 2011-03-29

Merge trunk

813. By Chuck Short on 2011-03-29

Catch the error that mount might through a bit better

814. By Chuck Short on 2011-03-29

Fix pep8 error

815. By Chuck Short on 2011-03-29

Fix up docstring

816. By Chuck Short on 2011-03-29

Style fixes

Unmerged revisions

=== modified file 'Authors'
--- Authors	2011-03-25 13:38:57 +0000
+++ Authors	2011-03-28 18:50:24 +0000
@@ -12,6 +12,7 @@
 Chmouel Boudjnah <[e-mail]>
 Chris Behrens <[e-mail]>
 Christian Berendt <[e-mail]>
+Chuck Short <[e-mail]>
 Cory Wright <[e-mail]>
 Dan Prince <[e-mail]>
 David Pravec <[e-mail]>

=== modified file 'nova/tests/test_virt.py'
--- nova/tests/test_virt.py	2011-03-25 00:16:53 +0000
+++ nova/tests/test_virt.py	2011-03-28 18:50:24 +0000
@@ -225,6 +225,49 @@
         self._check_xml_and_uri(instance_data, expect_kernel=True,
                                 expect_ramdisk=True, rescue=True)
 
+    def test_lxc_container_and_uri(self):
+        instance_data = dict(self.test_instance)
+        self._check_xml_and_container(instance_data)
+
+    def _check_xml_and_container(self, instance):
+        user_context = context.RequestContext(project=self.project,
+                                              user=self.user)
+        instance_ref = db.instance_create(user_context, instance)
+        host = self.network.get_network_host(user_context.elevated())
+        network_ref = db.project_get_network(context.get_admin_context(),
+                                             self.project.id)
+
+        fixed_ip = {'address':  self.test_ip,
+                    'network_id':   network_ref['id']}
+
+        ctxt = context.get_admin_context()
+        fixed_ip_ref = db.fixed_ip_create(ctxt, fixed_ip)
+        db.fixed_ip_update(ctxt, self.test_ip,
+                                 {'allocated':   True,
+                                  'instance_id': instance_ref['id']})
+
+        self.flags(libvirt_type='lxc')
+        conn = libvirt_conn.LibvirtConnection(True)
+
+        uri = conn.get_uri()
+        self.assertEquals(uri, 'lxc:///')
+
+        xml = conn.to_xml(instance_ref)
+        tree = xml_to_tree(xml)
+
+        check = [
+        (lambda t: t.find('.').get('type'), 'lxc'),
+        (lambda t: t.find('./os/type').text, 'exe'),
+        (lambda t: t.find('./devices/filesystem/target').get('dir'), '/')]
+
+        for i, (check, expected_result) in enumerate(check):
+            self.assertEqual(check(tree),
+                             expected_result,
+                             '%s failed common check %d' % (xml, i))
+
+        target = tree.find('./devices/filesystem/source').get('dir')
+        self.assertTrue(len(target) > 0)
+
     def _check_xml_and_uri(self, instance, expect_ramdisk, expect_kernel,
                            rescue=False):
         user_context = context.RequestContext(project=self.project,

=== modified file 'nova/virt/disk.py'
--- nova/virt/disk.py	2011-03-24 22:39:39 +0000
+++ nova/virt/disk.py	2011-03-28 18:50:24 +0000
@@ -116,6 +116,38 @@
         _unlink_device(device, nbd)
 
 
+def setup_container(image, container_dir=None):
+    """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)
+    out, 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:
+        out, err = utils.execute('sudo', 'losetup', '-a')
+        for loop in out.splitlines():
+            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 18:56:24 +0000
+++ nova/virt/libvirt.xml.template	2011-03-28 18:50:24 +0000
@@ -2,41 +2,48 @@
     <name>${name}</name>
     <memory>${memory_kb}</memory>
     <os>
-#if $type == 'uml'
-    #set $disk_prefix = 'ubd'
-    #set $disk_bus = 'uml'
-        <type>uml</type>
-        <kernel>/usr/bin/linux</kernel>
-        <root>/dev/ubda</root>
+#if $type == 'lxc'
+	#set $disk_prefix = ''
+	#set $disk_bus = ''
+		<type>exe</type>
+		<init>/sbin/init</init>
 #else
-    #if $type == 'xen'
-        #set $disk_prefix = 'sd'
-        #set $disk_bus = 'scsi'
-            <type>linux</type>
-            <root>/dev/xvda</root>
-    #else
-        #set $disk_prefix = 'vd'
-        #set $disk_bus = 'virtio'
-            <type>hvm</type>
-    #end if
-    #if $getVar('rescue', False)
-        <kernel>${basepath}/kernel.rescue</kernel>
-        <initrd>${basepath}/ramdisk.rescue</initrd>
-    #else
-        #if $getVar('kernel', None)
-            <kernel>${kernel}</kernel>
-            #if $type == 'xen'
-                <cmdline>ro</cmdline>
-            #else
-                <cmdline>root=/dev/vda console=ttyS0</cmdline>
-            #end if
-            #if $getVar('ramdisk', None)
-                <initrd>${ramdisk}</initrd>
-            #end if
-        #else
-            <boot dev="hd" />
-        #end if
-    #end if
+	#if $type == 'uml'
+		#set $disk_prefix = 'ubd'
+		#set $disk_bus = 'uml'
+			<type>uml</type>
+			<kernel>/usr/bin/linux</kernel>
+			<root>/dev/ubda</root>
+	#else
+		#if $type == 'xen'
+			#set $disk_prefix = 'sd'
+			#set $disk_bus = 'scsi'
+				<type>linux</type>
+				<root>/dev/xvda</root>
+		#else
+			#set $disk_prefix = 'vd'
+			#set $disk_bus = 'virtio'
+				<type>hvm</type>
+		#end if
+		#if $getVar('rescue', False)
+			<kernel>${basepath}/kernel.rescue</kernel>
+			<initrd>${basepath}/ramdisk.rescue</initrd>
+		#else
+			#if $getVar('kernel', None)
+				<kernel>${kernel}</kernel>
+				#if $type == 'xen'
+					<cmdline>ro</cmdline>
+				#else
+					<cmdline>root=/dev/vda console=ttyS0</cmdline>
+				#end if
+				#if $getVar('ramdisk', None)
+					<initrd>${ramdisk}</initrd>
+				#end if
+			 #else
+				<boot dev="hd" />
+			 #end if
+		#end if
+	#end if
 #end if
     </os>
     <features>
@@ -44,30 +51,37 @@
     </features>
     <vcpu>${vcpus}</vcpu>
     <devices>
-#if $getVar('rescue', False)
-        <disk type='file'>
-            <driver type='${driver_type}'/>
-            <source file='${basepath}/disk.rescue'/>
-            <target dev='${disk_prefix}a' bus='${disk_bus}'/>
-        </disk>
-        <disk type='file'>
-            <driver type='${driver_type}'/>
-            <source file='${basepath}/disk'/>
-            <target dev='${disk_prefix}b' bus='${disk_bus}'/>
-        </disk>
+#if $type == 'lxc'
+	<filesystem type='mount'>
+		<source dir='${basepath}/rootfs'/>
+		<target dir='/'/>
+	</filesystem>
 #else
-        <disk type='file'>
-            <driver type='${driver_type}'/>
-            <source file='${basepath}/disk'/>
-            <target dev='${disk_prefix}a' bus='${disk_bus}'/>
-        </disk>
-    #if $getVar('local', False)
-        <disk type='file'>
-            <driver type='${driver_type}'/>
-            <source file='${basepath}/disk.local'/>
-            <target dev='${disk_prefix}b' bus='${disk_bus}'/>
-        </disk>
-    #end if
+	#if $getVar('rescue', False)
+		    <disk type='file'>
+			    <driver type='${driver_type}'/>
+				<source file='${basepath}/disk.rescue'/>
+				<target dev='${disk_prefix}a' bus='${disk_bus}'/>
+			</disk>
+			<disk type='file'>
+				<driver type='${driver_type}'/>
+				<source file='${basepath}/disk'/>
+				<target dev='${disk_prefix}b' bus='${disk_bus}'/>
+			</disk>
+	#else
+			<disk type='file'>
+				<driver type='${driver_type}'/>
+				<source file='${basepath}/disk'/>
+				<target dev='${disk_prefix}a' bus='${disk_bus}'/>
+			</disk>
+		#if $getVar('local', False)
+			<disk type='file'>
+				<driver type='${driver_type}'/>
+				<source file='${basepath}/disk.local'/>
+				<target dev='${disk_prefix}b' bus='${disk_bus}'/>
+			</disk>
+		#end if
+#end if
 #end if
 
 #for $nic in $nics
@@ -77,7 +91,7 @@
             <!--   <model type='virtio'/>  CANT RUN virtio network right now -->
             <filterref filter="nova-instance-${name}-${nic.id}">
                 <parameter name="IP" value="${nic.ip_address}" />
-                <parameter name="DHCPSERVER" value="${nic.dhcp_server}" />
+                <parameter name="DHCPSERVER" value="${nic.dhcp_server}" />           
 #if $getVar('nic.extra_params', False)
                 ${nic.extra_params}
 #end if
@@ -87,7 +101,6 @@
             </filterref>
         </interface>
 #end for
-
         <!-- The order is significant here.  File must be defined first -->
         <serial type="file">
             <source path='${basepath}/console.log'/>

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-03-28 17:47:11 +0000
+++ nova/virt/libvirt_conn.py	2011-03-28 18:50:24 +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**
 
@@ -86,7 +86,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'
@@ -266,6 +266,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
@@ -344,6 +346,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)
 
@@ -625,6 +629,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
 
@@ -738,6 +745,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))
@@ -797,6 +808,9 @@
         if not inst['kernel_id']:
             target_partition = "1"
 
+        if FLAGS.libvirt_type == 'lxc':
+            target_partition = None
+
         key = str(inst['key_data'])
         net = None
 
@@ -842,6 +856,10 @@
                 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)
             except Exception as e:
                 # This could be a windows image, or a vmdk format disk
                 LOG.warn(_('instance %(inst_name)s: ignoring error injecting'