[Ovirt-devel] [PATCH server] Added support for booting a VM from an ISO image.

[Darryl Pierce]

Chris Lalancette wrote:
>> -  devs = [ 'hda', 'hdb', 'hdc', 'hdd' ]
>> -  i = 0
>> +  devs = [ 'hda', 'hdb', 'hdd', 'hde', 'hdf' ]
> 
> This can't work.  As it stands now, the qemu device model has 2 IDE controllers
> with a master/slave per controller, for a maximum of 4 devices.  That's
> generally what hardware has (had) as well, and nobody is really interested in
> pushing that up.  We really need to switch to using SCSI and/or virtio, but
> until we do, you can't just add an hdf; it has no chance of working.

Done. But, we'll want to make sure there's some logic added to the VM
creation page to disallow adding more than four storage devices.

>> +  which_device = 0
>>    diskDevices.each do |disk|
>> +    is_cdrom = (disk =~ /\.iso/) ? true : false
>> +    
>>      diskdev = Element.new("disk")
>> -    diskdev.add_attribute("type", "block")
>> -    diskdev.add_attribute("device", "disk")
>> -    diskdev.add_element("source", {"dev" => disk})
>> -    diskdev.add_element("target", {"dev" => devs[i]})
>> +    diskdev.add_attribute("type", is_cdrom ? "file" : "block")
>> +    diskdev.add_attribute("device", is_cdrom ? "cdrom" : "disk")
>> +
>> +    if is_cdrom
>> +      diskdev.add_element("readonly")
>> +      diskdev.add_element("source", {"file" => disk})
>> +      diskdev.add_element("target", {"dev" => "hdc", "bus" => "ide"})
>> +    else
>> +      diskdev.add_element("source", {"dev" => disk})
>> +      diskdev.add_element("target", {"dev" => devs[which_device]})
>> +    end
>> +
>>      doc.root.elements["devices"] << diskdev
>> -    i += 1
>> +    which_device += 1 unless is_cdrom
>>    end
> 
> This is really not the best way to do this, either.  The right thing to do here
> is to leave the loop pretty much as-is, but then to add the proper entries to
> the "devs" array in the caller.  That way this loop remains stupid, and anything
> smarter happens higher up in the chain.  That includes the "type", the "device",
> etc etc etc.  This also allows you to get rid of keying on ".iso" as whether
> this is a CD-ROM or not, which is a much better way to do it (it is kind of
> far-fetched, but there's no real reason your guest disk can't be on an NFS share
> and named my_guest.iso).

That was one of the issues I was troubling over is identifying which
disk is the image from Cobbler and which are just regular storage
devices from the pool. The trade off of doing the processing higher in
the callstack is that the higher code will now have to dig through the
VM's XML, figure out which device node is the image and then modify it.
Not sure if that's a better solution either.

>> +      unless found
>> +        puts "Creating a new NFS storage volume"
>> +        image_volume = StorageVolume.factory("NFS",
>> +          :filename => filename
>> +        )
>> +        
>> +        image_volume.storage_pool  
>> +        image_pool = find_storage_pool(ip_addr, export_path)
>> +        if image_pool
>> +          image_pool.storage_volumes << image_volume
>> +          image_pool.save!
>> +        end
>> +        vm.storage_volumes << image_volume
>> +        vm.save!
>> +      end
>> +    end
> 
> I'm not quite sure what's going on here.  It looks like you are looking for the
> storage volume to exist in the guest's list of storage volumes, and then if it
> doesn't exist, create it.  But this is not taskomatics job; the UI frontend
> should have added anything it cared about.  I'm not quite sure why you want
> taskomatic to do this, unless I am missing something and it is unavoidable
> because of Cobbler.

There's a subsequent call to connect_storage_devices which is calling
build to create any undefined storage pools. But, if it's better to do
that elsewhere, I can refactor the code and move that storage out to a
separate taskomatic call during the create_vm portion rather than the
start_vm portion.

> Also, since none of the preceding code requires "conn" to operate properly, I
> would move conn down to here.  I'm not sure if Ruby's garbage collector closes
> stray fd's automatically, but there's no reason to take a chance; just open up
> conn right before you need it.

Done. Good catch.

Any ideas on how to get this patch to have the VM actually boot from the
CDROM? That's the last big stumbling block I've been hitting. I have to
manually intervene to get the VM to boot from it.

-- 
Darryl L. Pierce <dpierce at redhat.com> : GPG KEYID: 6C4E7F1B
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dpierce.vcf
Type: text/x-vcard
Size: 319 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/ovirt-devel/attachments/20081008/a8289402/attachment.vcf>