Re: [Ovirt-devel] [PATCH]: Add NFS file creation/deletion to taskomatic

[Chris Lalancette <clalance redhat com>]

Chris Lalancette wrote:
> All,
>      Attached is a patch to implement NFS file creation/deletion in the
> taskomatic back-end.  Actually, this patch does quite a bit more than that:
> 
> 1)  Implement NFS file creation/deletion
> 2)  Fix LVM creation - due to the way I was testing, there was a glaring bug in
> the implementation that caused it not to work at all on new, raw LUNs
> 3)  Make sure to flip the Pool and Volume states to "available", once things are
> available.
> 
> With this patch in place, along with a minor patch to libvirt (updated packages
> coming soon), I was able to successfully create and delete LVM volumes, NFS
> files, and then use those newly created LUNs/files in a guest.
> 
> Caveat: note that NFS file creation can take a *long* time.  Basically, we fully
> allocate the file at creation time, which means we have to write the entire disk
> full of zeros.  During this time, both libvirtd (on the remote node) and
> taskomatic can't accept any more tasks.  This will eventually be fixed by a)
> making libvirtd multithreaded, and b) making taskomatic fully threaded.
> 
> Finally: with this in place, a couple of problems come to light.  One is that we
> probably want to give users the ability to choose a sparse file vs. a non-sparse
> file (but that's a minor change in the frontend and in taskomatic).  The second
> is much more complicated and important; namely, that we need users to be able to
> assign /dev/sda to iSCSI LUN 3, /dev/sdb to NFS file 2, etc.  At the *very*
> least we need users to be able to specify which device is the root device.  In
> any case, this is future work.

Updated patch, that addresses Scott's review comments.  There is also a small
bug fix in here where I choose hosts that are available and not disabled for
storage scanning, rather than any random host.

Signed-off-by: Chris Lalancette <clalance redhat com>

diff --git a/src/app/controllers/storage_controller.rb b/src/app/controllers/storage_controller.rb
index 6d171c9..148d1be 100644
--- a/src/app/controllers/storage_controller.rb
+++ b/src/app/controllers/storage_controller.rb
@@ -143,8 +143,9 @@ class StorageController < ApplicationController
       unless lvm_pool
         # FIXME: what should we do about VG/LV names?
         # for now auto-create VG name as ovirt_vg_#{ source_volume id}
-        lvm_pool = LvmStoragePool.new(:vg_name => "ovirt_vg_#{ source_volume id}",
-              :hardware_pool_id => @source_volume.storage_pool.hardware_pool_id)
+        new_params = { :vg_name => "ovirt_vg_#{ source_volume id}",
+          :hardware_pool_id => @source_volume.storage_pool.hardware_pool_id}
+        lvm_pool = StoragePool.factory(StoragePool::LVM, new_params)
         lvm_pool.source_volumes << @source_volume
         lvm_pool.save!
       end
diff --git a/src/app/models/lvm_storage_volume.rb b/src/app/models/lvm_storage_volume.rb
index 4aac265..8eb1f0e 100644
--- a/src/app/models/lvm_storage_volume.rb
+++ b/src/app/models/lvm_storage_volume.rb
@@ -25,4 +25,8 @@ class LvmStorageVolume < StorageVolume
   def volume_name
     "lv_name"
   end
+
+  def volume_create_params
+    return lv_name, size, lv_owner_perms, lv_group_perms, lv_mode_perms
+  end
 end
diff --git a/src/app/models/nfs_storage_volume.rb b/src/app/models/nfs_storage_volume.rb
index 2c18d67..61d5795 100644
--- a/src/app/models/nfs_storage_volume.rb
+++ b/src/app/models/nfs_storage_volume.rb
@@ -25,4 +25,8 @@ class NfsStorageVolume < StorageVolume
   def volume_name
     "filename"
   end
+
+  def volume_create_params
+    return filename, size, "0744", "0744", "0744"
+  end
 end
diff --git a/src/task-omatic/task_storage.rb b/src/task-omatic/task_storage.rb
index 7ef547b..19800fb 100644
--- a/src/task-omatic/task_storage.rb
+++ b/src/task-omatic/task_storage.rb
@@ -20,20 +20,6 @@ require 'utils'
 
 require 'libvirt'
 
-def build_libvirt_vol_xml(name, size, owner, group, mode)
-  vol_xml = Document.new
-  vol_xml.add_element("volume", {"type" => "logical"})
-  vol_xml.root.add_element("name").add_text(name)
-  vol_xml.root.add_element("capacity", {"unit" => "K"}).add_text(size.to_s)
-  vol_xml.root.add_element("target")
-  vol_xml.root.elements["target"].add_element("permissions")
-  vol_xml.root.elements["target"].elements["permissions"].add_element("owner").add_text(owner)
-  vol_xml.root.elements["target"].elements["permissions"].add_element("group").add_text(group)
-  vol_xml.root.elements["target"].elements["permissions"].add_element("mode").add_text(mode)
-
-  return vol_xml
-end
-
 def add_volumes_to_db(db_pool, libvirt_pool, owner = nil, group = nil, mode = nil)
   # FIXME: this is currently broken if you do something like:
   # 1.  Add an iscsi pool with 3 volumes (lun-1, lun-2, lun-3)
@@ -71,29 +57,29 @@ def add_volumes_to_db(db_pool, libvirt_pool, owner = nil, group = nil, mode = ni
     storage_volume.lv_owner_perms = owner
     storage_volume.lv_group_perms = group
     storage_volume.lv_mode_perms = mode
+    storage_volume.state = StorageVolume::STATE_AVAILABLE
     storage_volume.save!
   end
 end
 
-def storage_find_suitable_host(pool_id)
-  # find all of the hosts in the same pool as the storage
-  hosts = Host.find(:all, :conditions =>
-                    [ "hardware_pool_id = ?", pool_id ])
-
+def storage_find_suitable_host(hardware_pool)
   conn = nil
-  hosts.each do |host|
-    begin
-      # FIXME: this can actually hang up taskomatic for quite some time.  To
-      # see how, make one of your remote servers do "iptables -I INPUT -j DROP"
-      # and then try to run this; it will take TCP quite a while to give up.
-      # Unfortunately the solution is probably to do some sort of threading
-      conn = Libvirt::open("qemu+tcp://" + host.hostname + "/system")
-
-      # if we didn't raise an exception, we connected; get out of here
-      break
-    rescue Libvirt::ConnectionError
-      # if we couldn't connect for whatever reason, just try the next host
-      next
+  hardware_pool.hosts.each do |host|
+    if not host.is_disabled.nil? and host.is_disabled == 0 \
+      and host.state == Host::STATE_AVAILABLE
+      begin
+        # FIXME: this can hang up taskomatic for quite some time.  To see how,
+        # make one of your remote servers do "iptables -I INPUT -j DROP"
+        # and then try to run this; it will take TCP quite a while to give up.
+        # Unfortunately the solution is probably to do some sort of threading
+        conn = Libvirt::open("qemu+tcp://" + host.hostname + "/system")
+
+        # if we didn't raise an exception, we connected; get out of here
+        break
+      rescue Libvirt::ConnectionError
+        # if we couldn't connect for whatever reason, just try the next host
+        next
+      end
     end
   end
 
@@ -132,7 +118,7 @@ def refresh_pool(task)
     raise "Could not find storage pool"
   end
 
-  conn = storage_find_suitable_host(phys_db_pool.hardware_pool_id)
+  conn = storage_find_suitable_host(phys_db_pool.hardware_pool)
 
   begin
     phys_libvirt_pool = LibvirtPool.factory(phys_db_pool)
@@ -142,6 +128,9 @@ def refresh_pool(task)
       # OK, the pool is all set.  Add in all of the volumes
       add_volumes_to_db(phys_db_pool, phys_libvirt_pool)
 
+      phys_db_pool.state = StoragePool::STATE_AVAILABLE
+      phys_db_pool.save!
+
       # OK, now we've scanned the underlying hardware pool and added the
       # volumes.  Next we scan for pre-existing LVM volumes
       logical_xml = conn.discover_storage_pool_sources("logical")
@@ -211,45 +200,43 @@ end
 def create_volume(task)
   puts "create_volume"
 
-  lvm_db_volume = task.storage_volume
-  if lvm_db_volume == nil
+  db_volume = task.storage_volume
+  if db_volume == nil
     raise "Could not find storage volume to create"
   end
-  if lvm_db_volume[:type] != "LvmStorageVolume"
-    raise "The volume to create must be of type LvmStorageVolume, not type #{lvm_db_volume[:type]}"
-  end
 
-  lvm_db_pool = lvm_db_volume.storage_pool
-  if lvm_db_pool == nil
+  db_pool = db_volume.storage_pool
+  if db_pool == nil
     raise "Could not find storage pool"
   end
-  if lvm_db_pool[:type] != "LvmStoragePool"
-    raise "The pool for the volume must be of type LvmStoragePool, not type #{lvm_db_pool[:type]}"
-  end
 
-  conn = storage_find_suitable_host(lvm_db_pool.hardware_pool_id)
+  conn = storage_find_suitable_host(db_pool.hardware_pool)
 
   begin
-    phys_libvirt_pool = get_libvirt_pool_from_volume(lvm_db_volume)
-    phys_libvirt_pool.connect(conn)
+    if db_volume[:type] == "LvmStorageVolume"
+      phys_libvirt_pool = get_libvirt_lvm_pool_from_volume(db_volume)
+      phys_libvirt_pool.connect(conn)
+    end
 
     begin
-      lvm_libvirt_pool = LibvirtPool.factory(lvm_db_pool)
-      lvm_libvirt_pool.connect(conn)
+      libvirt_pool = LibvirtPool.factory(db_pool)
 
       begin
-        vol_xml = build_libvirt_vol_xml(lvm_db_volume.lv_name,
-                                        lvm_db_volume.size,
-                                        lvm_db_volume.lv_owner_perms,
-                                        lvm_db_volume.lv_group_perms,
-                                        lvm_db_volume.lv_mode_perms)
+        libvirt_pool.connect(conn)
 
-        lvm_libvirt_pool.create_vol_xml(vol_xml.to_s)
+        libvirt_pool.create_vol(*db_volume.volume_create_params)
+        db_volume.state = StorageVolume::STATE_AVAILABLE
+        db_volume.save!
+
+        db_pool.state = StoragePool::STATE_AVAILABLE
+        db_pool.save!
       ensure
-        lvm_libvirt_pool.shutdown
+        libvirt_pool.shutdown
       end
     ensure
-      phys_libvirt_pool.shutdown
+      if db_volume[:type] == "LvmStorageVolume"
+        phys_libvirt_pool.shutdown
+      end
     end
   ensure
     conn.close
@@ -259,34 +246,30 @@ end
 def delete_volume(task)
   puts "delete_volume"
 
-  lvm_db_volume = task.storage_volume
-  if lvm_db_volume == nil
-    raise "Could not find storage volume to delete"
-  end
-  if lvm_db_volume[:type] != "LvmStorageVolume"
-    raise "The volume to delete must be of type LvmStorageVolume, not type #{lvm_db_volume[:type]}"
+  db_volume = task.storage_volume
+  if db_volume == nil
+    raise "Could not find storage volume to create"
   end
 
-  lvm_db_pool = lvm_db_volume.storage_pool
-  if lvm_db_pool == nil
+  db_pool = db_volume.storage_pool
+  if db_pool == nil
     raise "Could not find storage pool"
   end
-  if lvm_db_pool[:type] != "LvmStoragePool"
-    raise "The pool for the volume must be of type LvmStoragePool, not type #{lvm_db_pool[:type]}"
-  end
 
-  conn = storage_find_suitable_host(lvm_db_pool.hardware_pool_id)
+  conn = storage_find_suitable_host(db_pool.hardware_pool)
 
   begin
-    phys_libvirt_pool = get_libvirt_pool_from_volume(lvm_db_volume)
-    phys_libvirt_pool.connect(conn)
+    if db_volume[:type] == "LvmStorageVolume"
+      phys_libvirt_pool = get_libvirt_lvm_pool_from_volume(db_volume)
+      phys_libvirt_pool.connect(conn)
+    end
 
     begin
-      lvm_libvirt_pool = LibvirtPool.factory(lvm_db_pool)
-      lvm_libvirt_pool.connect(conn)
+      libvirt_pool = LibvirtPool.factory(db_pool)
+      libvirt_pool.connect(conn)
 
       begin
-        libvirt_volume = lvm_libvirt_pool.lookup_vol_by_name(lvm_db_volume.lv_name)
+        libvirt_volume = libvirt_pool.lookup_vol_by_name(db_volume.read_attribute(db_volume.volume_name))
         # FIXME: we actually probably want to zero out the whole volume here, so
         # we aren't potentially leaking data from one user to another.  There
         # are two problems, though:
@@ -296,16 +279,22 @@ def delete_volume(task)
         # off another thread to do it
         libvirt_volume.delete
 
-        # FIXME: we really should be using lvm_db_volume.destroy here, but when
-        # I tried it I ran into some "Stale db reference" errors with
-        # ActiveRecord.  sseago thinks this could be indicative of a deeper
-        # error, so we need to investigate it more
-        LvmStorageVolume.delete(lvm_db_volume.id)
+        # Note: we have to nil out the task_target because when we delete the
+        # volume object, that also deletes all dependent tasks (including this
+        # one), which leads to accessing stale tasks.  Orphan the task, then
+        # delete the object; we can clean up orphans later (or not, depending
+        # on the audit policy)
+        task.task_target = nil
+        task.save!
+
+        db_volume.destroy
       ensure
-        lvm_libvirt_pool.shutdown
+        libvirt_pool.shutdown
       end
     ensure
-      phys_libvirt_pool.shutdown
+      if db_volume[:type] == "LvmStorageVolume"
+        phys_libvirt_pool.shutdown
+      end
     end
   ensure
     conn.close
diff --git a/src/task-omatic/task_vm.rb b/src/task-omatic/task_vm.rb
index 38aa7ab..35d78f2 100644
--- a/src/task-omatic/task_vm.rb
+++ b/src/task-omatic/task_vm.rb
@@ -74,7 +74,7 @@ def connect_storage_pools(conn, storage_volumes)
     # we have to special case LVM pools.  In that case, we need to first
     # activate the underlying physical device, and then do the logical one
     if volume[:type] == "LvmStorageVolume"
-      phys_libvirt_pool = get_libvirt_pool_from_volume(volume)
+      phys_libvirt_pool = get_libvirt_lvm_pool_from_volume(volume)
       phys_libvirt_pool.connect(conn)
     end
 
@@ -398,24 +398,36 @@ def start_vm(task)
       end
     end
 
-    conn = Libvirt::open("qemu+tcp://" + host.hostname + "/system")
-
     volumes = []
     volumes += vm.storage_volumes
     volumes << image_volume if image_volume
-    storagedevs = connect_storage_pools(conn, volumes)
-
-    # FIXME: get rid of the hardcoded bridge
-    xml = create_vm_xml(vm.description, vm.uuid, vm.memory_allocated,
-                        vm.memory_used, vm.num_vcpus_allocated, vm.boot_device,
-                        vm.vnic_mac_addr, "ovirtbr0", storagedevs)
 
-    dom = conn.define_domain_xml(xml.to_s)
-    dom.create
+    conn = Libvirt::open("qemu+tcp://" + host.hostname + "/system")
 
-    setVmVncPort(vm, dom)
+    begin
+      storagedevs = connect_storage_pools(conn, volumes)
 
-    conn.close
+      dom = nil
+      begin
+        # FIXME: get rid of the hardcoded bridge
+        xml = create_vm_xml(vm.description, vm.uuid, vm.memory_allocated,
+                            vm.memory_used, vm.num_vcpus_allocated,
+                            vm.boot_device, vm.vnic_mac_addr, "ovirtbr0",
+                            storagedevs)
+        dom = conn.define_domain_xml(xml.to_s)
+        dom.create
+
+        setVmVncPort(vm, dom)
+      rescue
+        if dom != nil
+          dom.undefine
+        end
+        teardown_storage_pools(conn)
+        raise ex
+      end
+    ensure
+      conn.close
+    end
   rescue => ex
     setVmState(vm, vm_orig_state)
     raise ex
diff --git a/src/task-omatic/taskomatic.rb b/src/task-omatic/taskomatic.rb
index bd7b5a4..ce37058 100755
--- a/src/task-omatic/taskomatic.rb
+++ b/src/task-omatic/taskomatic.rb
@@ -90,6 +90,8 @@ loop do
     get_credentials
 
     task.time_started = Time.now
+    task.state = Task::STATE_RUNNING
+    task.save!
 
     state = Task::STATE_FINISHED
     begin
@@ -123,6 +125,14 @@ loop do
     task.state = state
     task.time_ended = Time.now
     task.save!
+    puts "done"
+  end
+
+  # FIXME: here, we clean up "orphaned" tasks.  These are tasks that we had
+  # to orphan (set task_target to nil) because we were deleting the object they
+  # depended on.
+  Task.find(:all, :conditions => [ "task_target_id IS NULL and task_target_type IS NULL" ]).each do |task|
+    task.destroy
   end
   
   # we could destroy credentials, but another process might be using them (in
diff --git a/src/task-omatic/utils.rb b/src/task-omatic/utils.rb
index 26516ae..46a7e1b 100644
--- a/src/task-omatic/utils.rb
+++ b/src/task-omatic/utils.rb
@@ -13,7 +13,7 @@ def all_storage_pools(conn)
   return all_pools
 end
 
-def get_libvirt_pool_from_volume(db_volume)
+def get_libvirt_lvm_pool_from_volume(db_volume)
   phys_volume = StorageVolume.find(:first, :conditions =>
                                    [ "lvm_pool_id = ?", db_volume.storage_pool_id])
 
@@ -56,8 +56,8 @@ class LibvirtPool
 
     if @remote_pool == nil
       @remote_pool = conn.define_storage_pool_xml(@xml.to_s)
-      # we need this because we don't want to "build" LVM pools, which would
-      # destroy existing data
+      # we need this because we don't necessarily want to "build" LVM pools,
+      # which might destroy existing data
       if @build_on_start
         @remote_pool.build
       end
@@ -84,8 +84,16 @@ class LibvirtPool
     return @remote_pool.lookup_volume_by_name(name)
   end
 
-  def create_vol_xml(xml)
-    return @remote_pool.create_vol_xml(xml)
+  def create_vol(type, name, size, owner, group, mode)
+    @vol_xml = Document.new
+    @vol_xml.add_element("volume", {"type" => type})
+    @vol_xml.root.add_element("name").add_text(name)
+    @vol_xml.root.add_element("capacity", {"unit" => "K"}).add_text(size.to_s)
+    @vol_xml.root.add_element("target")
+    @vol_xml.root.elements["target"].add_element("permissions")
+    @vol_xml.root.elements["target"].elements["permissions"].add_element("owner").add_text(owner)
+    @vol_xml.root.elements["target"].elements["permissions"].add_element("group").add_text(group)
+    @vol_xml.root.elements["target"].elements["permissions"].add_element("mode").add_text(mode)
   end
 
   def shutdown
@@ -107,7 +115,21 @@ class LibvirtPool
     elsif pool[:type] == "NfsStoragePool"
       return NFSLibvirtPool.new(pool.ip_addr, pool.export_path)
     elsif pool[:type] == "LvmStoragePool"
-      return LVMLibvirtPool.new(pool.vg_name)
+      # OK, if this is LVM storage, there are two cases we need to care about:
+      # 1) this is a LUN with LVM already on it.  In this case, all we need to
+      #    do is to create a new LV (== libvirt volume), and be done with it
+      # 2) this LUN is blank, so there is no LVM on it already.  In this
+      #    case, we need to pvcreate, vgcreate first (== libvirt pool build),
+      #    and *then* create the new LV (== libvirt volume) on top of that.
+      #
+      # We can tell the difference between an LVM Pool that exists and one
+      # that needs to be created based on the value of the pool.state; 
+      # if it is PENDING_SETUP, we need to create it first
+      phys_volume = StorageVolume.find(:first, :conditions =>
+                                       [ "lvm_pool_id = ?", pool.id])
+
+      return LVMLibvirtPool.new(pool.vg_name, phys_volume.path,
+                                pool.state == StoragePool::STATE_PENDING_SETUP)
     else
       raise "Unknown storage pool type " + pool[:type].to_s
     end
@@ -151,6 +173,21 @@ class NFSLibvirtPool < LibvirtPool
     @xml.root.elements["target"].elements["path"].text = "/mnt/" + @name
   end
 
+  def create_vol(name, size, owner, group, mode)
+    # FIXME: this can actually take some time to complete (since we aren't
+    # doing sparse allocations at the moment).  During that time, whichever
+    # libvirtd we chose to use is completely hung up.  The solution is 3-fold:
+    # 1.  Allow sparse allocations in the WUI front-end
+    # 2.  Make libvirtd multi-threaded
+    # 3.  Make taskomatic multi-threaded
+    super("netfs", name, size, owner, group, mode)
+
+    # FIXME: we have to add the format as raw here because of a bug in libvirt;
+    # if you specify a volume with no format, it will crash libvirtd
+    @vol_xml.root.elements["target"].add_element("format", {"type" => "raw"})
+    @remote_pool.create_vol_xml(@vol_xml.to_s)
+  end
+
   def xmlequal?(docroot)
     return (docroot.attributes['type'] == @type and
             docroot.elements['source'].elements['host'].attributes['name'] == @host and
@@ -159,17 +196,23 @@ class NFSLibvirtPool < LibvirtPool
 end
 
 class LVMLibvirtPool < LibvirtPool
-  def initialize(vg_name)
+  def initialize(vg_name, device, build_on_start)
     super('logical', vg_name)
 
     @type = 'logical'
-    @build_on_start = false
+    @build_on_start = build_on_start
 
     @xml.root.elements["source"].add_element("name").add_text(@name)
+    @xml.root.elements["source"].add_element("device", {"path" => device})
 
     @xml.root.elements["target"].elements["path"].text = "/dev/" + @name
   end
 
+  def create_vol(name, size, owner, group, mode)
+    super("logical", name, size, owner, group, mode)
+    @remote_pool.create_vol_xml(@vol_xml.to_s)
+  end
+
   def xmlequal?(docroot)
     return (docroot.attributes['type'] == @type and
             docroot.elements['name'].text == @name and