[Ovirt-devel] [PATCH server 01/10] converted storage volume controller to use the service layer

[Scott Seago]

David Lutterkort wrote:
> On Tue, 2009-05-19 at 14:23 +0000, Scott Seago wrote:
>   
>> Signed-off-by: Scott Seago <sseago at redhat.com>
>> ---
>>  src/app/controllers/storage_volume_controller.rb |  164 +++++-----------------
>>  src/app/models/lvm_storage_volume.rb             |    8 +
>>  src/app/services/storage_volume_service.rb       |  145 +++++++++++++++++++
>>  3 files changed, 187 insertions(+), 130 deletions(-)
>>  create mode 100644 src/app/services/storage_volume_service.rb
>>     
>
> ACK, some musings that don't need to be addressed before committing:
>
>   
>> diff --git a/src/app/services/storage_volume_service.rb b/src/app/services/storage_volume_service.rb
>> new file mode 100644
>> index 0000000..6d338a5
>> --- /dev/null
>> +++ b/src/app/services/storage_volume_service.rb
>> +  # Load a new StorageVolume for creating
>> +  #
>> +  # === Instance variables
>> +  # [<tt>@storage_volume</tt>] loads a new StorageVolume object into memory
>> +  # [<tt>@storage_pool</tt>] Storage pool containing <tt>@storage_volume</tt>
>> +  # [<tt>@source_volume</tt>] Storage volume containing the LVM
>> +  #                           <tt>@storage_pool</tt> if storage type is LVM
>> +  # === Required permissions
>> +  # [<tt>Privilege::MODIFY</tt>] for the storage volume's HardwarePool
>> +  def svc_new(storage_pool_id, source_volume_id)
>>     
>
> I don't like how this method is used both for a new volume inside a
> storage pool, and for turning a volume into an LVM VG - from an API
> point of view it's fr from obvious what you need to pass in for either.
> I suspect that's a direct reflection of how the view passes in
> parameter. It would be good to clarify that API wise some, at a minimum
> with some comments on what to pass to trigger each behavior, or by
> turning this into two methods.
>
>   
Yes, I think I'll refactor this as a follow-on patch. I don't want to 
make any substantial changes to something early in this patch set as it 
might require re-testing everything that follows.

>> +    if storage_pool_id
>> +      @storage_pool = StoragePool.find(storage_pool_id)
>> +      unless @storage_pool.user_subdividable
>> +        raise ActionError.new("Unsupported action for " +
>> +                              "#{@storage_pool.get_type_label} volumes.")
>> +      end
>> +    else
>> +      @source_volume = StorageVolume.find(source_volume_id)
>> +      @storage_pool = @source_volume.storage_pool
>> +      unless @source_volume.supports_lvm_subdivision
>> +        raise ActionError.new("LVM is not supported for this storage volume")
>> +      end
>> +    end
>> +    authorized!(Privilege::MODIFY, at storage_pool.hardware_pool)
>> +
>> +    if source_volume_id
>> +      @storage_pool = @source_volume.lvm_storage_pool
>> +      unless @storage_pool
>> +        # FIXME: what should we do about VG/LV names?
>> +        # for now auto-create VG name as ovirt_vg_#{@source_volume.id}
>> +        new_params = { :vg_name => "ovirt_vg_#{@source_volume.id}",
>> +          :hardware_pool_id => @source_volume.storage_pool.hardware_pool_id}
>> +        @storage_pool = StoragePool.factory(StoragePool::LVM, new_params)
>> +        @storage_pool.source_volumes << @source_volume
>> +        @storage_pool.save!
>>     
>
> Doesn't that mean that a user creates a VG simply by opening the form,
> and that they might leave an unused VG behind if they never submit the
> form ? Not a big deal, but it requires that the rest of the code treats
> VG's as transient (e.g., when deleting @source_volume, we shouldn't
> generate an error if there is an unused VG on that volume)
>
>   
This is already handled. The "can we delete" logic checks the following:
    (lvm_storage_pool.nil? or lvm_storage_pool.storage_volumes.empty?)

I'm not a big fan of auto-creating this sort of thing in the 'new' form, 
but it seemed like things would be a lot more convoluted if we made the 
form work with or without an existing VG. Also, in this case there's not 
really any user-visible effect of the empty VG since we don't show the 
LVM VG in the UI.

Scott

> David
>
>
>