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

[Scott Seago]

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.
>
> Signed-off-by: Chris Lalancette <clalance at redhat.com>
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> Ovirt-devel mailing list
> Ovirt-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/ovirt-devel
> diff --git a/src/app/controllers/storage_controller.rb 
> b/src/app/controllers/storage_controller.rb
> index 6d171c9..2235b6a 100644
> --- a/src/app/controllers/storage_controller.rb
> +++ b/src/app/controllers/storage_controller.rb
> @@ -143,12 +143,19 @@ 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
>        new_volume_internal(lvm_pool, { :storage_pool_id => lvm_pool.id})
> +
> +      # now that we've created the new pool and volume, make sure to 
> link that
> +      # new LVM pool to the source volume
> +      @source_volume.lvm_pool_id = lvm_pool.id
> +      @source_volume.save!
> +
>        @storage_volume.lv_owner_perms='0744'
>        @storage_volume.lv_group_perms='0744'
>        @storage_volume.lv_mode_perms='0744'
You shouldn't have to set the lvm_pool_id for the source volume, as 
you've already set that relationship from the other side. When the 
lvm_pool was saved, the source_volume <-> lvm_pool association was saved 
into the database. Did you run into any particular problems without the 
latter code? From what I see here, we're essentially setting (and 
saving) that association twice.
> 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
Do we want to be able to set the permissions for all volume types? It 
seems a bit inconsistent to hard-code them for NFS but ask for them in 
the LVM form.

I haven't been able to test it yet -- my appliance build failed, and I 
don't have time to diagnose/fix that right now, but I'll see what I can 
do about that tomorrow morning. But we do need to get this in before we 
build -- so if this works with a fresh appliance build for you we should 
probably be able to push it.

Scott