[Ovirt-devel] [PATCH server] controller service layer refactoring for pools.

[David Lutterkort]

On Wed, 2009-04-29 at 16:50 -0400, Scott Seago wrote:
> David Lutterkort wrote:
> > On Tue, 2009-04-28 at 22:10 +0000, Scott Seago wrote:
> >   
> >> This is the second round of controller/service layer refactoring
> >> changes -- in this case for the pool-related controllers (HW, VM, and
> >> Smart pools)
> >>     
> >
> > This looks mostly good, modulo comments below. It would also be good to
> > include rdoc comments in the service modules to document required
> > permissions and instance variables that get set.
> >
> >   
> I'll look over your rdoc bits and then do something similar -- although 
> at this point, with this patch size (and considering vm_service already 
> pushed) that should probably be a follow-on patch.

Agreed.

> > Why the separate method render_show ? I don't think it simplifies things
> > compared to inlining that method.
> >
> >   
> render_show is used in the pool subclasses as well.

Ok .. didn't notice that when I wrote the above.

> Sure. I just fixed it here since it prevented me from testing the pool 
> controller refactoring properly, but I can pull it out assuming git 
> doesn't clobber my commit when I try to split it in two.

Interactive rebase is your friend ;) See [1] for more detailed
instructions on how to split a commit.

> >> +    # from before_filter
> >> +    @pool = HardwarePool.new(pool_hash)
> >> +    @parent = Pool.find(other_args[:parent_id])
> >> +    authorized!(Privilege::MODIFY, at parent)
> >> +
> >> +    alert = "Hardware Pool was successfully created."
> >> +    Pool.transaction do
> >> +      @pool.create_with_parent(@parent)
> >> +      begin
> >> +        if other_args[:resource_type] == "hosts"
> >> +          svc_move_hosts(@pool.id, other_args[:resource_ids].split(","), @pool.id)
> >> +        elsif other_args[:resource_type] == "storage"
> >> +          svc_move_storage(@pool.id, other_args[:resource_ids].split(","), @pool.id)
> >> +        end
> >> +        # wrapped in a transaction, so fail on partial success
> >> +      rescue PartialEuccessError => ex
> >> +        raise ActionError.new("Could not move all hosts or storage to this pool")
> >>     

In rereading this, I just noticed that there's a typo in
'PartialSuccessError'.

> > Why not make throwing PartialSuccessError part of the method's
> > signature ? Does wrapping in an ActionError really buy us anything ?
> >
> >   
> I'm wrapping it since I'm rethrowing it within a transaction -- causing 
> an abort.

My main objection was that by rethrowing a different exception we
obscure the actual failure for the caller and make it harder to create a
precise error message - this would actually all be moot if we made
PartialSuccessError a subclass of ActionError.

> >> +      end
> >> +    end
> >> +    return alert
> >> +  end
> >> +
> >> +  def svc_move_hosts(pool_id, host_ids, target_pool_id)
> >> +    svc_move_items_internal(pool_id, Host, host_ids, target_pool_id)
> >> +  end
> >> +  def svc_move_storage(pool_id, storage_pool_ids, target_pool_id)
> >> +    svc_move_items_internal(pool_id, StoragePool, storage_pool_ids, target_pool_id)
> >> +  end
> >> +  def svc_move_items_internal(pool_id, item_class, resource_ids, target_pool_id)
> >> +    # from before_filter
> >> +    @pool = HardwarePool.find(pool_id)
> >> +    target_pool = Pool.find(target_pool_id)
> >> +    authorized!(Privilege::MODIFY,target_pool)
> >> +    authorized!(Privilege::MODIFY, at pool) unless @pool == target_pool
> >>     
> >
> > Does it even make sense to do anything if @pool == target_pool ? We
> > should just bail.
> >
> >   
> We shouldn't bail, since for the "move hosts _to_ this pool" operation 
> they will be the same -- the unless clause keeps us from making the 
> check twice. For the "move hosts _to another_ pool" case the two will be 
> different.

Ahh, ok, that makes sense (though the unless clause is unnecessary with
the changes to set_perms that are also in the patch)

David

[1] http://book.git-scm.com/4_interactive_rebasing.html