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

[Chris Lalancette]

Scott Seago wrote:
>> 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.

I have to go back and look at it.  I'm pretty sure the lvm_pool_id wasn't being 
set properly, so I wasn't able to go from the given LVM volume -> LVM Pool -> 
source volume in taskomatic.

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

Yeah, it's a little inconsistent.  I didn't like this wart either.  Honestly, at 
the moment, what we should do is:

1)  Remove the ability to change the permissions from the LVM UI (since it is 
basically meaningless at the moment).
2)  Just make all volumes, regardless of type, default to 0744.

Later on, when we come up with a better idea for permissions, we can implement 
it the right way.

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

OK.  I haven't actually tried yet with a fresh build; I'll do that next, just to 
make sure things work out of the box.

-- 
Chris Lalancette