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

[David Lutterkort]

On Wed, 2009-05-20 at 11:18 -0400, Scott Seago wrote:
> David Lutterkort wrote:
> > 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.

Agreed.

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

Yeah, not a big deal, just something we need to be careful about.

David