[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Darryl L. Pierce wrote:
> Signed-off-by: Darryl L. Pierce <dpierce redhat com>
> ---
>  src/app/controllers/vm_controller.rb |   13 ++++--
>  src/app/models/vm.rb                 |   31 ++++++++++++-
>  src/task-omatic/task_vm.rb           |   85 ++++++++++++++++++++++++++++-----
>  src/test/unit/vm_test.rb             |   56 +++++++++++++++++++++-
>  4 files changed, 163 insertions(+), 22 deletions(-)
> 
> diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb
> index f5c0845..8b16c94 100644
> --- a/src/app/controllers/vm_controller.rb
> +++ b/src/app/controllers/vm_controller.rb
> @@ -223,13 +223,18 @@ class VmController < ApplicationController
>    def _setup_provisioning_options
>      @provisioning_options = [[Vm::PXE_OPTION_LABEL, Vm::PXE_OPTION_VALUE],
>                               [Vm::HD_OPTION_LABEL, Vm::HD_OPTION_VALUE]]
> -    # FIXME add cobbler images too
> +
>      begin
> +      @provisioning_options += Cobbler::Image.find.collect do |image|
> +        [image.name + Vm::COBBLER_IMAGE_SUFFIX,
> +          "#{Vm::IMAGE_PREFIX} #{Vm::COBBLER_PREFIX}#{Vm::PROVISIONING_DELIMITER}#{image.name}"]
> +      end
> +
>        @provisioning_options += Cobbler::Profile.find.collect do |profile|
>          [profile.name + Vm::COBBLER_PROFILE_SUFFIX,
> -         Vm::COBBLER_PREFIX + Vm::PROVISIONING_DELIMITER +
> -         Vm::PROFILE_PREFIX + Vm::PROVISIONING_DELIMITER + profile.name]
> -      end
> +          "#{Vm::PROFILE_PREFIX} #{Vm::COBBLER_PREFIX}#{Vm::PROVISIONING_DELIMITER}#{profile.name}"]
> +
> +    end
>      rescue
>        #if cobbler doesn't respond/is misconfigured/etc just don't add profiles
>      end
> diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
> index ace6fb1..963da84 100644
> --- a/src/app/models/vm.rb
> +++ b/src/app/models/vm.rb
> @@ -49,7 +49,7 @@ class Vm < ActiveRecord::Base
>    PROFILE_PREFIX         = "profile"
>    IMAGE_PREFIX           = "image"
>    COBBLER_PROFILE_SUFFIX = " (Cobbler Profile)"
> -  COBBLER_IMAGE_SUFFIX   = " (Cobbler Profile)"
> +  COBBLER_IMAGE_SUFFIX   = " (Cobbler Image)"
>  
>    PXE_OPTION_LABEL       = "PXE Boot"
>    PXE_OPTION_VALUE       = "pxe"
> @@ -139,7 +139,12 @@ class Vm < ActiveRecord::Base
>    end
>  
>    def provisioning_and_boot_settings=(settings)
> -    if settings==PXE_OPTION_VALUE
> +    # if the settings have a prefix that matches cobber settings, then process
> +    # those details
> +    if settings =~ /\ #{COBBLER_PREFIX}/
> +      self[:boot_device] = BOOT_DEV_NETWORK
> +      self[:provisioning] = settings
> +    elsif settings==PXE_OPTION_VALUE
>        self[:boot_device]= BOOT_DEV_NETWORK
>        self[:provisioning]= nil
>      elsif settings==HD_OPTION_VALUE
> @@ -242,6 +247,28 @@ class Vm < ActiveRecord::Base
>      vm_resource_pool.search_users
>    end
>  
> +  # Reports whether the VM is uses Cobbler for booting.
> +  #
> +  def uses_cobbler?
> +    (self.provisioning != nil) && (self.provisioning.include? COBBLER_PREFIX)
> +  end
> +
> +  # Returns the cobbler type.
> +  #
> +  def cobbler_type
> +    if self.uses_cobbler?
> +      self.provisioning[/^(.*)@/,1]
> +    end
> +  end
> +
> +  # Returns the cobbler provisioning name.
> +  #
> +  def cobbler_name
> +    if self.uses_cobbler?
> +      self.provisioning[/^ *  *:(.*)/,1]
> +    end
> +  end
> +
>    protected
>    def validate
>      resources = vm_resource_pool.max_resources_for_vm(self)
> diff --git a/src/task-omatic/task_vm.rb b/src/task-omatic/task_vm.rb
> index 3588224..0c1be3a 100644
> --- a/src/task-omatic/task_vm.rb
> +++ b/src/task-omatic/task_vm.rb
> @@ -65,16 +65,26 @@ def create_vm_xml(name, uuid, memAllocated, memUsed, vcpus, bootDevice,
>    doc.root.elements["devices"].add_element("emulator")
>    doc.root.elements["devices"].elements["emulator"].text = "/usr/bin/qemu-kvm"
>  
> -  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.

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

>  
>    doc.root.elements["devices"].add_element("interface", {"type" => "bridge"})
> @@ -154,15 +164,12 @@ def create_vm(task)
>    # create cobbler system profile
>    begin
>      if vm.provisioning and !vm.provisioning.empty?
> -      provisioning_arr = vm.provisioning.split(Vm::PROVISIONING_DELIMITER)
> -      if provisioning_arr[0]==Vm::COBBLER_PREFIX
> -        if provisioning_arr[1]==Vm::PROFILE_PREFIX
> +      if vm.uses_cobbler?
> +        if vm.cobbler_type == Vm::PROFILE_PREFIX:
>            system = Cobbler::System.new('name' => vm.uuid,
>                                         'profile' => provisioning_arr[2])
>            system.interfaces=[Cobbler::NetworkInterface.new({'mac_address' => vm.vnic_mac_addr})]
>            system.save
> -        elsif provisioning_arr[1]==Vm::IMAGE_PREFIX
> -          #FIXME handle cobbler images
>          end
>        end
>      end
> @@ -231,6 +238,22 @@ def shutdown_vm(task)
>    setVmShutdown(vm)
>  end
>  
> +# Find thes storage pool with the given ip address and export path.
> +#
> +def find_storage_pool(ip_addr, export_path)
> +  result = StoragePool.find(:first, 
> +    :conditions => 
> +      ['ip_addr = ? and export_path = ?',ip_addr, export_path])
> +
> +  if result
> +    puts "Found #{ip_addr}:#{export_path}"
> +  else
> +    puts "FUCK!"

Heh.  Well, you want to get rid of these puts; either raise an exception here
and let the higher level print out your exception, or just be quiet and let the
caller decide what to do if result == nil.

> +  end
> +  
> +  return result
> +end
> +
>  def start_vm(task)
>    puts "start_vm"
>  
> @@ -268,8 +291,44 @@ def start_vm(task)
>  
>      conn = Libvirt::open("qemu+tcp://" + host.hostname + "/system")
>  
> -    storagedevs = connect_storage_pools(conn, vm)
> +    # if the VM is an image, then prepare things
> +    if vm.uses_cobbler? && (vm.cobbler_type == Vm::IMAGE_PREFIX)
> +      details = Cobbler::Image.find_one(vm.cobbler_name)
> +      raise Exception.new("Image #{vm.cobbler_name} not found in Cobbler server") unless details
> +
> +      ignored, ip_addr, export_path, filename =
> +        details.file.split(/(.*):(.*)\/(.*)/)
>  
> +      found = false
> +
> +      vm.storage_volumes.each do |volume|
> +        if volume.filename == filename
> +          if (volume.storage_pool.ip_addr == ip_addr) &&
> +          (volume.storage_pool.export_path == export_path)
> +            found = true
> +          end
> +        end
> +      end
> +
> +      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.

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.

-- 
Chris Lalancette


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]