[Ovirt-devel] [PATCH server] Revised attempt at refactoring the vm_controller using the service layer.

[David Lutterkort]

On Wed, 2009-04-22 at 22:54 -0400, Scott Seago wrote:
> David Lutterkort wrote:
> >
> > The one thing that sticks out is the 'delete' method - that should just
> > repeatedly call svc_destroy (wrapping the deletion in a txn is somewhat
> > bogus here, the user should be ok with having only part of the list of
> > VM's deleted)
> >
> >   
> Yes, I think I inserted a comment around that. I would have done it 
> except for one problem -- repeated calls to svc_delete would repeatedly 
> re-set @vm and re-check permissions. My thinking was that we should add 
> a delete_vms action to the VM pool controller, since we only ever delete 
> an array of VMs from a single VM pool. The VM pool is the object that we 
> check permission on anyway, so the idea would be that we provide a 
> method on the VM pool controller to delete a list of VMs that belong to 
> that VM pool.

Why not make use of the fact that we already cache the curent VM object
and change set_perms to

  def set_perms(perm_obj)
    return if @perm_obj && @perm_obj.id == perm_obj.id
    ...
  end

That effectively caches permission checks; of course, we might still
incur one load of a HW pool per VM, but I wouldn't sweat that too much
right now.

> So I think we should leave this as-is for this patch but when 
> resources_controller is converted to use the service layer we should 
> pull the delete_vms action into it. It really doesn't work so well as a 
> single action on the VM service since we're acting on multiple VMs.

I think with the above we can do that with very little work; we might
eventually need a delete_vms service method, but for now, just looping
should be good enough.

> >
> > As we talked about on IRC, longer term it would be better if the
> > @can_XXX attributes became methods on the service.
> >
> > I also think the can_XXX methods on pool should be removed and we should
> > have something like the following in AppService:
> >
> >         def user
> >           @user ||= get_login_user
> >         end
> >         
> >         def has_privilege(priv, perm_obj = nil)
> >           perm_obj ||= @perm_obj
> >           perm_obj.has_privilege(user, priv)
> >         end
> >           
> >         def can_view(perm_obj = nil)
> >           has_privilege(Privilege::VIEW, perm_obj)
> >         end
> >
> >   
> How will this work from the Views? Right now the only place we access 
> @can_XXX is in the views, but generally views only have access to 
> controller instance variables, not methods..
> But certianly the can_XXX methods on pool could go away.

You can 'export' controller methods by marking them has helper_method,
e.g. 'helper_method :can_view' in ApplicationController should do the
trick.

> >> diff --git a/src/app/views/vm/_form.rhtml b/src/app/views/vm/_form.rhtml
> >> index 58a3d61..610f2bc 100644
> >> --- a/src/app/views/vm/_form.rhtml
> >> +++ b/src/app/views/vm/_form.rhtml
> >> @@ -3,8 +3,7 @@
> >>  <%  start_resources = @vm.vm_resource_pool.available_resources_for_vm(@vm) %>
> >>  
> >>  <!--[form:vm]-->
> >> -<%= hidden_field 'vm', 'vm_resource_pool_id' unless @vm_resource_pool_name %>
> >> -<%= hidden_field_tag 'vm_resource_pool_name', @vm_resource_pool_name if @vm_resource_pool_name %>
> >> +<%= hidden_field 'vm', 'vm_resource_pool_id' %>
> >>  <%= hidden_field_tag 'hardware_pool_id', @hardware_pool.id if @hardware_pool %>
> >>  
> >>      <%= text_field_with_label "Name:", "vm", "description", {:style=>"width:250px;"}  %>
> >>     
> >
> > Could this go into a separate commit, too ?
> >
> > David
> >
> >   
> Well it could --  but it would require a fair amount of disentangling.

Ok .. not a big deal.

David