Merge lp:~justin-fathomdb/nova/raw-disk-images into lp:~hudson-openstack/nova/trunk

[justinsb]

Add support for disk images that are 'raw', i.e. don't need a kernel or ramdisk. This makes it simpler to have Windows VMs, or to use VirtualBox as the host. To remain EC2 API compatible, if you specify the kernel as aki-00000000, this triggers the 'raw disk image' behaviour.

[Jay Pipes (jaypipes)]

Hi!

Good stuff, Justin! ++ for long option names :)

A couple tiny nits, though :)

27 + if not os.path.exists(mapped_device):
28 + raise exception.Error('Mapped device was not found: %s' % mapped_device)

There is a NotFound exception class in /nova/exception.py that may be better that raising the generic error here. Or, alternately, you could raise IOError, as that is what is raised by open() when the file does not exist?

77 + # This is correct for Ubuntu, but might not be right for other distros
78 + rulesfile = os.path.join(fs, 'etc', 'udev', 'rules.d', '70-persistent-net.rules')
79 + yield execute('rm -f %s' % rulesfile)

Best to add a TODO(justinsb) there so it's noted for future improvements :)

Finally, not sure about making Cheetah a dependency here...what do other think?

Cheers!
jay

review: Needs Fixing

[justinsb (justin-fathomdb)]

Thanks for the review Jay.

The 'mapped device not found' exception is actually tricky; it is expected if we can't read the partitions after loopback, which happens if we have a non-raw-disk-image (i.e. a 'sparse' format). The test is to try to make that case obvious. I'm not sure what we should do here, but currently the caller (at least for virtualbox) ignores errors when trying to inject the machine image. I've therefore left it as a raw exception.Error, but clarified the message to make the intent clearer.

I've added the TODO. Again, there's a big picture question of what we should be injecting and how we should be injecting it; hopefully this will become clearer as we start launching more OSes and the guest agent shapes up (which is required for Windows, I believe)

As for Cheetah, I have no particular reason to choose Cheetah vs any other templating system - it looked like decent syntax, the project seemed not-dead, and there's an Ubuntu package. I would suggest that we do need _a_ templating system, for complex templating or optional inclusions (here I use it so that the kernel and ramdisk XML elements are only output if they are needed). What that templating system is, I don't really care. The one datapoint I have is that Cheetah "works for me", but if anyone else has more experience...

[Jay Pipes (jaypipes)]

> The 'mapped device not found' exception is actually tricky; it is expected if
> we can't read the partitions after loopback, which happens if we have a non-
> raw-disk-image (i.e. a 'sparse' format). The test is to try to make that case
> obvious. I'm not sure what we should do here, but currently the caller (at
> least for virtualbox) ignores errors when trying to inject the machine image.
> I've therefore left it as a raw exception.Error, but clarified the message to
> make the intent clearer.

OK, understood. I would still recommend making a new exception class in /nova/exception.py (MappedDeviceNotFound?) possibly inherited from IOError. Just makes the code a little cleaner, that's all.

> I've added the TODO. Again, there's a big picture question of what we should
> be injecting and how we should be injecting it; hopefully this will become
> clearer as we start launching more OSes and the guest agent shapes up (which
> is required for Windows, I believe)

Cool :)

> As for Cheetah, I have no particular reason to choose Cheetah vs any other
> templating system - it looked like decent syntax, the project seemed not-dead,
> and there's an Ubuntu package. I would suggest that we do need _a_ templating
> system, for complex templating or optional inclusions (here I use it so that
> the kernel and ramdisk XML elements are only output if they are needed). What
> that templating system is, I don't really care. The one datapoint I have is
> that Cheetah "works for me", but if anyone else has more experience...

Yeah, I'm not sure, which is why I asked others for opinions. I'm not familiar with Python's templating frameworks/libraries.

-jay

[Soren Hansen (soren)]

I'm cool with Cheetah. I've used it before and I don't know of another templating system that's supposed to be the canonical one.

As for the udev network thing: Ubuntu will refrain from storing the MAC->interface-name mapping if the MAC address is "locally administered". nova.utils.generate_mac() generates such a MAC. If you can somehow get VirtualBox to accept that MAC, you should be able to remove that hack.

> @defer.inlineCallbacks
> -def inject_data(image, key=None, net=None, partition=None, execute=None):
> +def inject_data( image, key=None, net=None, dns=None,
> + remove_network_udev=False,
> + partition=None, execute=None):

PEP-8 says not to put whitespace immediately inside parentheses.

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py 2010-08-18 21:14:24 +0000
+++ nova/virt/libvirt_conn.py 2010-08-24 09:44:45 +0000
@@ -260,13 +275,28 @@
     def toXml(self, instance):
         # TODO(termie): cache?
         logging.debug("Starting the toXML method")
+ template_contents = open(FLAGS.libvirt_xml_template).read()
         xml_info = instance.datamodel.copy()
         # TODO(joshua): Make this xml express the attached disks as well

Since I added UML capabilities to Nova, the template to use is already
put into self.libvirt_xml in __init__. Please use that instead, as
it's based on a different template if libvirt_type is "uml". Also, the
changes you made to libvirt.qemu.xml.template need to be done similarly
for libvirt.uml.xml.template.

Other than that, it looks good, and I'm really looking forward to seeing
this land! :)

review: Needs Fixing

[Vish Ishaya (vishvananda)]

Justin, did we lose you? I'd like to get this merged, but it looks like there are a few fixes requested.

[termie (termie)]

I'll just weigh in here on my thoughts about Cheetah, mainly that I think it is bad to allow much logic in the template code. Agreed that we all need conditionals, but seeing 'getVar' in there really throws some flags for me.

A much much smaller but powerful and declarative-style templating engine that I've grown to prefer is called jsontemplate, http://json-template.googlecode.com/svn/trunk/doc/Introducing-JSON-Template.html

Our usage is pretty minimal here so the syntax isn't shockingly different but in Cheetah

#if $getVar('kernel', None)
        <kernel>${kernel}</kernel>
    #if $getVar('ramdisk', None)
        <initrd>${ramdisk}</initrd>
    #end if
        <cmdline>root=/dev/vda1 console=ttyS0</cmdline>
#end if

and

template_contents = open(FLAGS.libvirt_xml_template).read()
str(Template(template_contents, searchList=[ xml_info ] ))

becomes

{.section kernel}
        <kernel>{kernel}</kernel>
    {.section ramdisk}
        <initrd>{ramdisk}</initrd>
    {.end}
        <cmdline>root=/dev/vda1 console=ttyS0</cmdline>
{.end}

and

jsontemplate.FromFile(FLAGS.libvirt_xml_template).expand(xml_info)

The other benefits are that the engine is a single file and easily extensible with formatters

The "json" part of it comes from the fact that it was designed and implemented in python and javascript at the same time.

There are a variety of style nits (extra spaces before or around things, importing objects instead of modules, using tabs instead of spaces in the xml), so please take another scan over the code to resolve those :)

Other than those discussion points looks like a great addition to our core functionality :)

review: Needs Fixing

[Rick Clark (dendrobates)]

If Justin is willing to fix this up. I am fine with moving forward with cheetah. We can have an in depth discussions about templating engines at the summit if we want.

Today is the last day to get any preexisting feature merge requests merged.
The code base has changed quite a bit since this was proposed. It may be a difficult task to get it done today, but if Justin is will to do the work today, we will merge it.

[justinsb (justin-fathomdb)]

Resubmitting.

Cleaned up the formatting issues in the code, minimized the patch to remove unnecessary stuff. Using Cheetah (pending review at the summit), and Cheetah-ified the other libvirt ____.xml.template files.

I left the exception for when the mounted image isn't found as-is for now. I think when we're ready to handle it better, then we can throw more informative exceptions throughout the disk-injection code. This should probably be when we figure out what to do about Windows images or other not-easily-injectable images.

[Jay Pipes (jaypipes)]

Looks good, Justin. I'm cool with Cheetah. We can always discuss templating stuff at the summit.

One thing that looked odd, though:

263 + using_kernel = inst.kernel_id and True
264 + if using_kernel:

wouldn't it be simpler to put:

if inst.kernel_id:

or even:

if inst['kernel_id']:

since I think we're moving away from object dot notation to dict-notation.

Just a thought. :)

-jay

review: Approve

[lp:~justin-fathomdb/nova/raw-disk-images]

166. By Justin Santa Barbara <justinsb@justinsb-desktop> on 2010-10-15

Removed 'and True' oddity

[justinsb (justin-fathomdb)]

Thanks Jay - I removed the 'and True'. Not sure where I got that idea from... I think I was trying to force it to a boolean, but experimentation shows that wasn't the case!

I left the inst.kernel_id (as opposed to inst['kernel_id']) because it seems that there's a mix of styles in the file, but kernel_id is using the dot notation. I don't think this is the right time in the release cycle for style patches. Is the next release branch open yet?

[Jay Pipes (jaypipes)]

On Fri, Oct 15, 2010 at 2:12 PM, justinsb <email address hidden> wrote:
> Thanks Jay - I removed the 'and True'.  Not sure where I got that idea from... I think I was trying to force it to a boolean, but experimentation shows that wasn't the case!

:)

> I left the inst.kernel_id (as opposed to inst['kernel_id']) because it seems that there's a mix of styles in the file, but kernel_id is using the dot notation.

No worries at all. We'll be discussing standardizing the returned
data "objects" at the summit...

> I don't think this is the right time in the release cycle for style patches.  Is the next release branch open yet?

Sure, agreed. And no, next release branch is not open yet...we're
trying to fix a weird bug that's happening on Hudson at the
moment...I'll ping you when it's ready to merge into.
-jay

[Vish Ishaya (vishvananda)]

Are we trying to get this into austin? Or are we waiting until next release?

[Vish Ishaya (vishvananda)]

Soren suggested that we just use whole disk by default if no kernel or ramdisk is specified, instead of using the default kernel and ramdisk. If an image needs a kernel and ramdisk to function, it can specify one in the image json. I'm happy to have this merged and make that change later though.

Also, has this been pep8ed?

[termie (termie)]

Only tiny bits (besides thinking cheetah is overkill, stylistically non-pep8 and generally a gun to shoot ourselves in the foot with... but summit, or something):

Extra space before "it's" and I'd claim extra space after "images."

51 + # We can only loopback mount raw images. If the device isn't there,
52 + # it's normally because it's a .vmdk or a .vdi etc

Extra space after the comma:

94 +DEFINE_string('null_kernel', 'aki-00000000',
95 + 'kernel image that indicates not to use a kernel, '
96 + ' but to use a raw disk image instead')

Extra spaces around "xml_info" and after the end-square-bracket:

325 + xml = str(Template(self.libvirt_xml, searchList=[ xml_info ] ))

review: Needs Fixing

[David Pravec (alekibango)]

> Soren suggested that we just use whole disk by default if no kernel or ramdisk
> is specified, instead of using the default kernel and ramdisk.
+1

[Thierry Carrez (ttx)]

This is superseded by https://code.launchpad.net/~soren/nova/raw-disk-images/+merge/43712

Unmerged revisions

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2010-10-14 06:26:58 +0000
+++ nova/api/ec2/cloud.py	2010-10-15 18:07:52 +0000
@@ -797,9 +797,15 @@
         kernel_id = kwargs.get('kernel_id', kernel_id)
         ramdisk_id = kwargs.get('ramdisk_id', ramdisk_id)
 
+        if kernel_id == str(FLAGS.null_kernel):
+            kernel_id = None
+            ramdisk_id = None
+            
         # make sure we have access to kernel and ramdisk
-        images.get(context, kernel_id)
-        images.get(context, ramdisk_id)
+        if kernel_id:
+            images.get(context, kernel_id)
+        if ramdisk_id:
+            images.get(context, ramdisk_id)
 
         logging.debug("Going to run %s instances...", num_instances)
         launch_time = time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime())
@@ -826,8 +832,8 @@
         base_options = {}
         base_options['state_description'] = 'scheduling'
         base_options['image_id'] = image_id
-        base_options['kernel_id'] = kernel_id
-        base_options['ramdisk_id'] = ramdisk_id
+        base_options['kernel_id'] = kernel_id or ''
+        base_options['ramdisk_id'] = ramdisk_id or ''
         base_options['reservation_id'] = reservation_id
         base_options['key_data'] = key_data
         base_options['key_name'] = kwargs.get('key_name', None)

=== modified file 'nova/compute/disk.py'
--- nova/compute/disk.py	2010-08-16 12:16:21 +0000
+++ nova/compute/disk.py	2010-10-15 18:07:52 +0000
@@ -96,7 +96,7 @@
     If partition is not specified it mounts the image as a single partition.
 
     """
-    out, err = yield execute('sudo losetup -f --show %s' % image)
+    out, err = yield execute('sudo losetup --find --show %s' % image)
     if err:
         raise exception.Error('Could not attach image to loopback: %s' % err)
     device = out.strip()
@@ -110,6 +110,15 @@
                                                    partition)
         else:
             mapped_device = device
+
+        # We can only loopback mount raw images.  If the device isn't there,
+        #  it's normally because it's a .vmdk or a .vdi etc
+        if not os.path.exists(mapped_device):
+            raise exception.Error(
+                'Mapped device was not found (we can only inject raw disk images): %s'
+                % mapped_device)
+
+        # Configure ext2fs so that it doesn't auto-check every N boots
         out, err = yield execute('sudo tune2fs -c 0 -i 0 %s' % mapped_device)
 
         tmpdir = tempfile.mkdtemp()
@@ -137,12 +146,12 @@
                 yield execute('sudo kpartx -d %s' % device)
     finally:
         # remove loopback
-        yield execute('sudo losetup -d %s' % device)
+        yield execute('sudo losetup --detach %s' % device)
 
 
 @defer.inlineCallbacks
 def _inject_key_into_fs(key, fs, execute=None):
-    sshdir = os.path.join(os.path.join(fs, 'root'), '.ssh')
+    sshdir = os.path.join(fs, 'root', '.ssh')
     yield execute('sudo mkdir -p %s' % sshdir) # existing dir doesn't matter
     yield execute('sudo chown root %s' % sshdir)
     yield execute('sudo chmod 700 %s' % sshdir)
@@ -152,7 +161,6 @@
 
 @defer.inlineCallbacks
 def _inject_net_into_fs(net, fs, execute=None):
-    netfile = os.path.join(os.path.join(os.path.join(
-            fs, 'etc'), 'network'), 'interfaces')
+    netfile = os.path.join(fs, 'etc', 'network', 'interfaces')
     yield execute('sudo tee %s' % netfile, net)
 

=== modified file 'nova/flags.py'
--- nova/flags.py	2010-10-01 20:06:14 +0000
+++ nova/flags.py	2010-10-15 18:07:52 +0000
@@ -201,6 +201,9 @@
               'default ramdisk to use, testing only')
 DEFINE_string('default_instance_type', 'm1.small',
               'default instance type to use, testing only')
+DEFINE_string('null_kernel', 'aki-00000000',
+              'kernel image that indicates not to use a kernel, '
+              ' but to use a raw disk image instead')
 
 DEFINE_string('vpn_image_id', 'ami-CLOUDPIPE', 'AMI for cloudpipe vpn server')
 DEFINE_string('vpn_key_suffix',

=== modified file 'nova/virt/libvirt.qemu.xml.template'
--- nova/virt/libvirt.qemu.xml.template	2010-09-27 11:13:29 +0000
+++ nova/virt/libvirt.qemu.xml.template	2010-10-15 18:07:52 +0000
@@ -1,24 +1,28 @@
-<domain type='%(type)s'>
-    <name>%(name)s</name>
+<domain type='${type}'>
+    <name>${name}</name>
     <os>
         <type>hvm</type>
-        <kernel>%(basepath)s/kernel</kernel>
-        <initrd>%(basepath)s/ramdisk</initrd>
+#if $getVar('kernel', None)
+        <kernel>${kernel}</kernel>
+    #if $getVar('ramdisk', None)
+        <initrd>${ramdisk}</initrd>
+    #end if
         <cmdline>root=/dev/vda1 console=ttyS0</cmdline>
+#end if
     </os>
     <features>
         <acpi/>
     </features>
-    <memory>%(memory_kb)s</memory>
-    <vcpu>%(vcpus)s</vcpu>
+    <memory>${memory_kb}</memory>
+    <vcpu>${vcpus}</vcpu>
     <devices>
         <disk type='file'>
-            <source file='%(basepath)s/disk'/>
+            <source file='${disk}'/>
             <target dev='vda' bus='virtio'/>
         </disk>
         <interface type='bridge'>
-            <source bridge='%(bridge_name)s'/>
-            <mac address='%(mac_address)s'/>
+            <source bridge='${bridge_name}'/>
+            <mac address='${mac_address}'/>
             <!--   <model type='virtio'/>  CANT RUN virtio network right now -->
             <filterref filter="nova-instance-%(name)s">
                 <parameter name="IP" value="%(ip_address)s" />
@@ -26,7 +30,7 @@
             </filterref>
         </interface>
         <serial type="file">
-            <source path='%(basepath)s/console.log'/>
+            <source path='${basepath}/console.log'/>
             <target port='1'/>
         </serial>
     </devices>

=== modified file 'nova/virt/libvirt.uml.xml.template'
--- nova/virt/libvirt.uml.xml.template	2010-09-27 11:13:29 +0000
+++ nova/virt/libvirt.uml.xml.template	2010-10-15 18:07:52 +0000
@@ -1,26 +1,26 @@
-<domain type='%(type)s'>
-    <name>%(name)s</name>
-    <memory>%(memory_kb)s</memory>
+<domain type='${type}'>
+    <name>${name}</name>
+    <memory>${memory_kb}</memory>
     <os>
-        <type>%(type)s</type>
+        <type>${type}</type>
         <kernel>/usr/bin/linux</kernel>
         <root>/dev/ubda1</root>
     </os>
     <devices>
         <disk type='file'>
-            <source file='%(basepath)s/disk'/>
+            <source file='${disk}'/>
             <target dev='ubd0' bus='uml'/>
         </disk>
         <interface type='bridge'>
-            <source bridge='%(bridge_name)s'/>
-            <mac address='%(mac_address)s'/>
-            <filterref filter="nova-instance-%(name)s">
-                <parameter name="IP" value="%(ip_address)s" />
-                <parameter name="DHCPSERVER" value="%(dhcp_server)s" />
+            <source bridge='${bridge_name}'/>
+            <mac address='${mac_address}'/>
+            <filterref filter="nova-instance-${name}">
+                <parameter name="IP" value="${ip_address}" />
+                <parameter name="DHCPSERVER" value="${dhcp_server}" />
             </filterref>
         </interface>
         <console type="file">
-            <source path='%(basepath)s/console.log'/>
+            <source path='${basepath}/console.log'/>
         </console>
     </devices>
 </domain>

=== modified file 'nova/virt/libvirt.xen.xml.template'
--- nova/virt/libvirt.xen.xml.template	2010-09-20 09:33:35 +0000
+++ nova/virt/libvirt.xen.xml.template	2010-10-15 18:07:52 +0000
@@ -1,28 +1,33 @@
-<domain type='%(type)s'>
-    <name>%(name)s</name>
+<domain type='${type}'>
+    <name>${name}</name>
     <os>
         <type>linux</type>
-        <kernel>%(basepath)s/kernel</kernel>
-        <initrd>%(basepath)s/ramdisk</initrd>
+#if $getVar('kernel', None)
+        <kernel>${kernel}</kernel>
+    #if $getVar('ramdisk', None)
+        <initrd>${ramdisk}</initrd>
+    #end if
+        <cmdline>root=/dev/vda1 console=ttyS0</cmdline>
+#end if
         <root>/dev/xvda1</root>
         <cmdline>ro</cmdline>
     </os>
     <features>
         <acpi/>
     </features>
-    <memory>%(memory_kb)s</memory>
-    <vcpu>%(vcpus)s</vcpu>
+    <memory>${memory_kb}</memory>
+    <vcpu>${vcpus}</vcpu>
     <devices>
         <disk type='file'>
-            <source file='%(basepath)s/disk'/>
+            <source file='${disk}'/>
             <target dev='sda' />
         </disk>
         <interface type='bridge'>
-            <source bridge='%(bridge_name)s'/>
-            <mac address='%(mac_address)s'/>
+            <source bridge='${bridge_name}'/>
+            <mac address='${mac_address}'/>
         </interface>
         <console type="file">
-            <source path='%(basepath)s/console.log'/>
+            <source path='${basepath}/console.log'/>
             <target port='1'/>
         </console>
     </devices>

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2010-10-14 05:51:55 +0000
+++ nova/virt/libvirt_conn.py	2010-10-15 18:07:52 +0000
@@ -43,6 +43,8 @@
 from nova.compute import power_state
 from nova.virt import images
 
+from Cheetah.Template import Template
+
 libvirt = None
 libxml2 = None
 
@@ -325,16 +327,26 @@
 
         if not os.path.exists(basepath('disk')):
            yield images.fetch(inst.image_id, basepath('disk-raw'), user, project)
-        if not os.path.exists(basepath('kernel')):
-           yield images.fetch(inst.kernel_id, basepath('kernel'), user, project)
-        if not os.path.exists(basepath('ramdisk')):
-           yield images.fetch(inst.ramdisk_id, basepath('ramdisk'), user, project)
+
+        using_kernel = inst.kernel_id
+        if using_kernel:
+            if not os.path.exists(basepath('kernel')):
+                yield images.fetch(inst.kernel_id, basepath('kernel'), user, project)
+            if not os.path.exists(basepath('ramdisk')):
+                yield images.fetch(inst.ramdisk_id, basepath('ramdisk'), user, project)
 
         execute = lambda cmd, process_input=None: \
                   process.simple_execute(cmd=cmd,
                                          process_input=process_input,
                                          check_exit_code=True)
 
+        # For now, we assume that if we're not using a kernel, we're using a 
+        # partitioned disk image where the target partition is the first 
+        # partition
+        target_partition = None
+        if not using_kernel:
+            target_partition = "1"
+        
         key = str(inst['key_data'])
         net = None
         network_ref = db.network_get_by_instance(context.get_admin_context(),
@@ -355,10 +367,19 @@
             if net:
                 logging.info('instance %s: injecting net into image %s',
                     inst['name'], inst.image_id)
-            yield disk.inject_data(basepath('disk-raw'), key, net, execute=execute)
+            try:
+                yield disk.inject_data(basepath('disk-raw'), key, net,
+                                       partition=target_partition, 
+                                       execute=execute)
+            except Exception as e:
+                # This could be a windows image, or a vmdk format disk
+                logging.warn('instance %s: ignoring error injecting data'
+                             ' into image %s (%s)',
+                             inst['name'], inst.image_id, e)
 
-        if os.path.exists(basepath('disk')):
-            yield process.simple_execute('rm -f %s' % basepath('disk'))
+        if using_kernel:
+            if os.path.exists(basepath('disk')):
+                yield process.simple_execute('rm -f %s' % basepath('disk'))
 
         bytes = (instance_types.INSTANCE_TYPES[inst.instance_type]['local_gb']
                  * 1024 * 1024 * 1024)
@@ -390,10 +411,22 @@
                     'mac_address': instance['mac_address'],
                     'ip_address': ip_address,
                     'dhcp_server': dhcp_server }
-        libvirt_xml = self.libvirt_xml % xml_info
+        
+        if xml_info['kernel_id']:
+            xml_info['kernel'] = xml_info['basepath'] + "/kernel"
+
+        if xml_info['ramdisk_id']:
+            xml_info['ramdisk'] = xml_info['basepath'] + "/ramdisk"
+
+        if xml_info['ramdisk_id'] or xml_info['kernel_id']:
+            xml_info['disk'] = xml_info['basepath'] + "/disk"
+        else:
+            xml_info['disk'] = xml_info['basepath'] + "/disk-raw"
+
+        xml = str(Template(self.libvirt_xml, searchList=[ xml_info ] ))
         logging.debug('instance %s: finished toXML method', instance['name'])
 
-        return libvirt_xml
+        return xml
 
     def get_info(self, instance_name):
         virt_dom = self._conn.lookupByName(instance_name)

=== modified file 'tools/pip-requires'
--- tools/pip-requires	2010-10-01 18:02:51 +0000
+++ tools/pip-requires	2010-10-15 18:07:52 +0000
@@ -2,6 +2,7 @@
 pep8==0.5.0
 pylint==0.19
 IPy==0.70
+Cheetah==2.4.2.1
 M2Crypto==0.20.2
 amqplib==0.6.1
 anyjson==0.2.4