[et-mgmt-tools] VM images

[Mark McLoughlin]

Hi David,
	Looks pretty cool, some comments in now particular order:

  - At first I thought the pygrub thing isn't needed - but you're right,
    for PV it's not enough to say "boot this image from its MBR", you
    need to say "the assumptions that pygrub makes about there being a 
    grub config file etc. will hold true"

  - Including vcpu, memory, graphics and nic in this metadata is mixing 
    up two things - the things the image need in order to boot and the 
    defaults recommended when instantiating a guest with the image. 
    Perhaps put them in a different toplevel element e.g.

      <image>
        <boot_options>
           <boot>
           ...
        </boot_options>
        <storage>
            <disk>
            ...
        </storage>
        <defaults>
          <memory>
          <vcpus>
          ...
        </defaults>
      </image>

  - The disk -> target mapping thing is pretty strange - I'd suggest 
    that the order the disks are listed in just specifies the target.

    I think I see your problem, though, you've got 2 boot disks and you 
    need to make sure the HVM disk is first under HVM. How about if you 
    could just specify the filename of the disk image to boot from in 
    <loader> ... the logic to get the libvirt XML right is a bit hairy, 
    but should be doable. Is it hd/cdrom, make sure the boot disk is 
    the first disk etc.

      <loader disk="boot-hvm.img" />

  - You don't have a human readable name for a UI that allows people to 
    choose from a number of images.

  - From what I can see, you still have the ability to create a disk at 
    instantiation time, but not format it?

  - If you added ImageInstaller support to virt-install, couldn't 
    virt-image almost just run virt-install rather than re-implementing 
    a lot of it? i.e. at a glance, it looks like you've forked 
    virt-install in order to have a version with a simpler command 
    line. I don't have a problem with virt-image, and the simpler 
    command line, it's the copied and pasted code I don't like.

   - This looks odd:

+    order = [ "xen", "kvm", "kqemu", "qemu" ]
+    for o in order:
+        if types.count(o) > 0:

     i.e. it'd be better not to have to keep that list updated. How 
     about you just pick the first type? If libvirt doesn't order the 
     list of available types in a useful order, maybe it should?

   - You don't have a way of specifying the disk image should be 
     read-only.

   - Shouldn't we be copying even system disk images, unless they're 
     read-only, on instantiation - i.e. you should be able to run 
     virt-image multiple times.

Cheers,
Mark.