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

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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ovirt-taskomatic-nfs-file-create.patch
Type: text/x-patch
Size: 17199 bytes
Desc: not available
URL:
<http://listman.redhat.com/archives/ovirt-devel/attachments/20081125/ad507780/attachment.bin>

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

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 at redhat.com>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ovirt-taskomatic-nfs-file-create-v2.patch
URL:
<http://listman.redhat.com/archives/ovirt-devel/attachments/20081126/57b7ce07/attachment.ksh>