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

[David Lutterkort]

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.

> +    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)

David