Merge lp:~johannes.erdfelt/nova/bug723996 into lp:~hudson-openstack/nova/trunk

[Johannes Erdfelt]

Add new flag 'max_kernel_ramdisk_size' to specify a maximum size of kernel or ramdisk so we don't copy large files to dom0 and fill up /boot/guest

[Brian Waldon (bcwaldon)]

Looks good. Are there any other related image sizes we want to check with regards to this bug?

review: Approve

[Brian Waldon (bcwaldon)]

Actually, now that I think about it, would it make sense to add support for other virt drivers?

[Johannes Erdfelt (johannes.erdfelt)]

On Tue, Apr 12, 2011, Brian Waldon <email address hidden> wrote:
> Actually, now that I think about it, would it make sense to add support
> for other virt drivers?

Depends on if they need to store kernels and ramdisks on the VM host
filesystem like XenServer does.

I'm not familiar enough with other virt drivers to know if they need
similar checks.

JE

[Devin Carlen (devcamcar)]

Not a Xen expert but this looks pretty reasonable. Minor grammar/style nit:

21 + _("Kernel/Ramdisk image is too large, %(vdi_size)d bytes "
22 + "(max %(max_size)d bytes)") %

Should be:

"Kernel/Ramdisk image is too large: %(vdi_size)d bytes "

(colon instead of comma)

review: Approve

[lp:~johannes.erdfelt/nova/bug723996]

979. By Johannes Erdfelt on 2011-04-13

Minor formatting cleanup

[Rick Harris (rconradharris)]

Looks good, thanks Johannes.

Some thoughts:

* I noticed that FLAGS seem to be split between vm_utils.py and xenapi_conn.py. Is there a semantic distinction or was that more-or-less arbitrary. If possible, I'd like to see all of the flags go in one place; in this case, xenapi_conn since that's where most already exist.

Should we move the newly-created flag there and create a bug to move the other two flags?

* Should we log the fact that an excessively large kernel was attempted? Basically, extracting the msg part of the exception and then logging it before raising:

  msg = _("helpful message here")
  LOG.error(msg)
  raise exception.Error(msg)

This is a pretty common pattern; wonder if a util.raise_with_logging(exc_class, msg) helper would be useful?

Just a couple of suggestions, will leave up to you whether to update the patch. Thanks again.

review: Approve

[Cory Wright (corywright)]

Johannes, would you like to respond to Rick's comments from above?

review: Needs Information

[lp:~johannes.erdfelt/nova/bug723996]

980. By Johannes Erdfelt on 2011-05-09

Merge with trunk

[Johannes Erdfelt (johannes.erdfelt)]

The FLAGS are defined where they are used. I don't see why changing it is better.

I'm pretty sure exceptions get logged as is. Perhaps I don't understand what the advantage of logging the error twice is?

[Vish Ishaya (vishvananda)]

going to merge this. Rick, if you want to add the exception logging thing lets do it in another patch

review: Approve

=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2011-04-26 22:48:28 +0000
+++ nova/virt/xenapi/vm_utils.py	2011-05-09 15:52:50 +0000
@@ -48,6 +48,8 @@
 flags.DEFINE_string('default_os_type', 'linux', 'Default OS type')
 flags.DEFINE_integer('block_device_creation_timeout', 10,
                      'time to wait for a block device to be created')
+flags.DEFINE_integer('max_kernel_ramdisk_size', 16 * 1024 * 1024,
+                     'maximum size in bytes of kernel or ramdisk images')
 
 XENAPI_POWER_STATE = {
     'Halted': power_state.SHUTDOWN,
@@ -444,6 +446,12 @@
         if image_type == ImageType.DISK:
             # Make room for MBR.
             vdi_size += MBR_SIZE_BYTES
+        elif image_type == ImageType.KERNEL_RAMDISK and \
+             vdi_size > FLAGS.max_kernel_ramdisk_size:
+            max_size = FLAGS.max_kernel_ramdisk_size
+            raise exception.Error(
+                _("Kernel/Ramdisk image is too large: %(vdi_size)d bytes, "
+                  "max %(max_size)d bytes") % locals())
 
         name_label = get_name_label_for_image(image)
         vdi_ref = cls.create_vdi(session, sr_ref, name_label, vdi_size, False)