[Ovirt-devel] [PATCH server] controller service layer refactoring for pools.
Scott Seago
sseago at redhat.com
Wed Apr 29 20:50:41 UTC 2009
Comments to the comments -- in case further discussion is warranted. I'm
going to start making the obvious changes later today or tomorrow morning.
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.
>
>> @@ -17,8 +17,10 @@
>> # MA 02110-1301, USA. A copy of the GNU General Public License is
>> # also available at http://www.gnu.org/copyleft/gpl.html.
>> #
>> +require 'services/hardware_pool_service'
>>
>
> This breaks class reloading in the development environment, and is not
> necessary at all. Should be removed
>
>
Ahh, OK. I wasn't sure if it was needed since it's not a standard rails
dir. I'm trying to remember why we had that for utils, but if it works
without it I'll pull that out.
>> @@ -113,8 +112,13 @@ class HardwareController < PoolController
>> end
>>
>> def show_storage
>> - @storage_tree = @pool.storage_tree(:filter_unavailable => false, :include_used => true).to_json
>> - show
>> + begin
>> + svc_show(params[:id])
>> + @storage_tree = @pool.storage_tree(:filter_unavailable => false, :include_used => true).to_json
>> + render_show
>> + rescue PermissionError => perm_error
>> + handle_auth_error(perm_error.message)
>> + end
>> end
>>
>
> I know that it's just moving code around, but please avoid lines with
> more than 80 chars.
>
>
OK. this one's easy enough to fix.
>> @@ -190,144 +194,49 @@ class HardwareController < PoolController
>>
>
>
>> def add_hosts
>> - edit_items(Host, :move_hosts, @pool.id, :add)
>> + edit_items(Host, @pool.id, :add)
>> end
>>
>> def move_hosts
>> - edit_items(Host, :move_hosts, params[:target_pool_id], :move)
>> + edit_items(Host, params[:target_pool_id], :move)
>> end
>>
>> def add_storage
>> - edit_items(StoragePool, :move_storage, @pool.id, :add)
>> + edit_items(StoragePool, @pool.id, :add)
>> end
>>
>> def move_storage
>> - edit_items(StoragePool, :move_storage, params[:target_pool_id], :move)
>> + edit_items(StoragePool, params[:target_pool_id], :move)
>> end
>>
>> - #FIXME: we need permissions checks. user must have permission on src pool
>> - # in addition to the current pool (which is checked). We also need to fail
>> - # for storage that aren't currently empty
>> - def edit_items(item_class, item_method, target_pool_id, item_action)
>> - resource_ids_str = params[:resource_ids]
>> - resource_ids = resource_ids_str.split(",").collect {|x| x.to_i}
>> -
>> - # if user doesn't have modify permission on both source and destination
>> - unless @pool.can_modify(@user) and Pool.find(target_pool_id).can_modify(@user)
>> - render :json => { :success => false,
>> - :alert => "Cannot #{item_action.to_s} #{item_class.table_name.humanize} without admin permissions on both pools" }
>> - return
>> - end
>> -
>> - # relay error message if movable check fails for any resource
>> - success = true
>> - failed_resources = ""
>> - resource_ids.each {|x|
>> - unless item_class.find(x).movable?
>> - success = false
>> - failed_resources += x.to_s + " "
>> - end
>> - }
>> - resource_ids.delete_if { |x| ! item_class.find(x).movable? }
>> -
>> + def edit_items(item_class, target_pool_id, item_action)
>> begin
>> - @pool.transaction do
>> - @pool.send(item_method, resource_ids, target_pool_id)
>> + if item_class == Host
>> + alert = svc_move_hosts(params[:id], params[:resource_ids].split(","), target_pool_id)
>> + elsif item_class == StoragePool
>> + alert = svc_move_storage(params[:id], params[:resource_ids].split(","), target_pool_id)
>> + else
>> + raise "invalid class #{item_class}"
>> end
>> - rescue
>> - success = false
>> - end
>> -
>> - if success
>> - render :json => { :success => true,
>> - :alert => "#{item_action.to_s} #{item_class.table_name.humanize} successful.",
>> + render :json => { :success => true, :alert => alert,
>> :storage => @pool.storage_tree({:filter_unavailable => false, :include_used => true, :state => item_action.to_s})}
>> - else
>> - render :json => { :success => false,
>> - :alert => "#{item_action.to_s} #{item_class.table_name.humanize} failed" +
>> - (failed_resources == "" ? "." : " for " + failed_resources) }
>> + rescue PermissionError => perm_error
>> + handle_auth_error(perm_error.message)
>> + # If we need to give more details as to which hosts/storage succeeded,
>> + # they're in the exception
>> + rescue PartialSuccessError => error
>> + render :json => { :success => false, :alert => error.message }
>> + rescue Exception => ex
>> + render :json => { :success => false, :alert => error.message }
>> end
>> end
>>
>
> This is a good example of how our current error handling makes things
> more complex: we have move_hosts/move_storage actions, call a generic
> edit_items, and then dispatch in that method on the class of one of the
> parameters.
>
> First off, it would be more rubyish if edit_items took a block that has
> the proper calls to svc_move_hosts/svc_move_storage. Second, it seems
> the main point of edit_items is to factor all the error handling
> overhead intoa common method.
>
> For the time being, edit_items should take a block; in the future we
> need to look into handling more errors with rescue_with, avoiding all
> that error handling clutter altogether.
>
>
OK I'll look into adding a block now, although we still use item_class
for messages -- so adding the block would result in more duplication I'd
think. Since I'll have to add the block with the right svc call for each
of the above methods based on the item_class. On one hand it would look
more ruby-ish -- on the other hand, the change would violate DRY, as I'd
be passing in the same information in two different ways.
>> diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb
>> index 06f8768..41c1be9 100644
>> --- a/src/app/controllers/pool_controller.rb
>> +++ b/src/app/controllers/pool_controller.rb
>>
>
>
>> @@ -40,6 +37,15 @@ class PoolController < ApplicationController
>> end
>>
>> def show
>> + begin
>> + svc_show(params[:id])
>> + render_show
>> + rescue PermissionError => perm_error
>> + handle_auth_error(perm_error.message)
>> + end
>> + end
>> +
>> + def render_show
>> respond_to do |format|
>> format.html {
>> render :layout => 'tabs-and-content' if params[:ajax]
>> @@ -49,10 +55,15 @@ class PoolController < ApplicationController
>> render :xml => @pool.to_xml(XML_OPTS)
>> }
>> end
>> - end
>>
>
> 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.
>> @@ -97,30 +108,111 @@ class PoolController < ApplicationController
>>
>
>
>> + rescue Exception => ex
>> + respond_to do |format|
>> + format.json { json_error("pool", @pool, ex) }
>> + format.xml { render :xml => @pool.errors,
>> + :status => :unprocessable_entity }
>> + end
>> + end
>>
>
> While I'm on a tear about error handling, I really don't like seeing
> 'rescue Exception' or even worse a bare rescue. It would be nice if we
> could finda way to report errors for json and xml with additional error
> templates.
>
>
We can hopefully fix this when we go to rescue_from, but for the json
(and possibly xml) output we have to catch all exceptions and return an
actual json response.
>> + end
>> +
>> + def update
>> + begin
>> + alert = svc_update(params[:id], params[:pool] ? params[:pool] :
>> + params[:hardware_pool])
>> + respond_to do |format|
>> + format.html {
>> + reply = { :object => "pool", :success => true, :alert => alert }
>> + render :json => reply
>> + }
>>
>
> Is that right ? format.html and render :json ?
>
>
Good catch. This was not intentional.
>> diff --git a/src/app/controllers/resources_controller.rb b/src/app/controllers/resources_controller.rb
>> index c61ef46..63bc98c 100644
>> --- a/src/app/controllers/resources_controller.rb
>> +++ b/src/app/controllers/resources_controller.rb
>> @@ -16,15 +16,15 @@
>> # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>> # MA 02110-1301, USA. A copy of the GNU General Public License is
>> # also available at http://www.gnu.org/copyleft/gpl.html.
>> +require 'services/vm_resource_pool_service'
>>
>
> Not needed
>
>
Got it.
>> @@ -69,89 +69,62 @@ class ResourcesController < PoolController
>>
>
> ...
>
>
>> - def destroy
>> - if @pool.destroy
>> - alert="Virtual Machine Pool was successfully deleted."
>> - success=true
>> - else
>> - alert="Failed to delete virtual machine pool."
>> - success=false
>> + success = failures.empty?
>> + alert = ""
>> + if !successes.empty?
>> + alert == "Virtual Machine Pools #{successes.collect{|pool| pool.name}.join(', ')} were successfully deleted."
>> + end
>> + if !failures.empty?
>> + alert == " Errors in deleting VM Pools #{failures.collect{|pool,err| "#{pool.name}: #{err}"}.join(', ')}."
>> end
>>
>
> s/==/=/
>
> The logic also deserves a comment that clobbering alert on failure is
> intentional.
>
>
Actually the logic deserves to be fixed such that I'm appending to alert
-- which was the intent. :-)
>> diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb
>> index fb6ccb5..bc20024 100644
>> --- a/src/app/controllers/smart_pools_controller.rb
>> +++ b/src/app/controllers/smart_pools_controller.rb
>> @@ -17,14 +17,12 @@
>> # MA 02110-1301, USA. A copy of the GNU General Public License is
>> # also available at http://www.gnu.org/copyleft/gpl.html.
>> #
>> +require 'services/smart_pool_service'
>>
>
> Not needed.
>
>
right.
>> diff --git a/src/app/controllers/storage_volume_controller.rb b/src/app/controllers/storage_volume_controller.rb
>> index b6b0593..d4a2561 100644
>> --- a/src/app/controllers/storage_volume_controller.rb
>> +++ b/src/app/controllers/storage_volume_controller.rb
>>
>
> This seems tangential to the pool refactoring - could that go into a
> separate patch ?
>
>
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.
>> diff --git a/src/app/models/host.rb b/src/app/models/host.rb
>> index 06d7388..0665c3f 100644
>> --- a/src/app/models/host.rb
>> +++ b/src/app/models/host.rb
>> @@ -134,6 +134,10 @@ class Host < ActiveRecord::Base
>> hardware_pool.search_users
>> end
>>
>> + def permission_obj
>> + hardware_pool
>> + end
>> +
>>
>
> Nice !
>
>
>> diff --git a/src/app/services/application_service.rb b/src/app/services/application_service.rb
>> index 4dc5eba..251b37a 100644
>> --- a/src/app/services/application_service.rb
>> +++ b/src/app/services/application_service.rb
>> @@ -29,18 +29,34 @@
>> module ApplicationService
>> class PermissionError < RuntimeError; end
>> class ActionError < RuntimeError; end
>> + class PartialSuccessError < RuntimeError
>> + def initialize(msg, failures={}, successes=[])
>> + @failures = failures
>> + @successes = successes
>> + end
>> + def failures
>> + @failures
>> + end
>> + def successes
>> + @successes
>> + end
>> + end
>>
>
> attr_reader :failures, :successes ?
>
>
heh. yeah that would be better.
>> # Including class must provide a GET_LOGIN_USER
>>
>> + def user
>> + @user ||= get_login_user
>> + end
>> +
>>
>
> I don't think this is the right place to cache the user - if we want
> that, it should be done in get_login_user. In any event, this and the
> changes to set_perm_obj should be in a separate patch.
>
Yeah it would be better to cache that elsewhere. I can probably separate
this since I don't _think_ any of these things are required for the
multi-object operation.
>> diff --git a/src/app/services/hardware_pool_service.rb b/src/app/services/hardware_pool_service.rb
>> new file mode 100644
>> index 0000000..0dbe8dc
>> --- /dev/null
>> +++ b/src/app/services/hardware_pool_service.rb
>> @@ -0,0 +1,125 @@
>> +#
>> +# Copyright (C) 2009 Red Hat, Inc.
>> +# Written by Scott Seago <sseago at redhat.com>,
>> +# David Lutterkort <lutter at redhat.com>
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; version 2 of the License.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write to the Free Software
>> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>> +# MA 02110-1301, USA. A copy of the GNU General Public License is
>> +# also available at http://www.gnu.org/copyleft/gpl.html.
>> +# Mid-level API: Business logic around HW pools
>> +module HardwarePoolService
>> +
>> + include PoolService
>> +
>> + def svc_create(pool_hash, other_args)
>>
>
> How about 'svc_create(parent_id, resource_types, resource_ids,
> pool_hash)' ? The signature feels a little WUI specific.
>
>
Sure, that makes sense. I'll do that.
>> + # 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")
>>
>
> 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. The thinking here is that the create action with passed-in
resources is generally reached via the 'move' UI -- a user picks some
hosts or storage and instead of moving them to an existing pool, the
'move to new pool' action is clicked, so this 'new pool' form exists for
the purpose of moving some resources to a new pool. In this case, I was
thinking that failure to move these resources should abort the
transaction and _not_ create the new pool either. So since we're
treating 'create' as an all-or-nothing operation (vs. 'move these
things' where partial completion makes sense) I'm catching
PartialSuccessError and re-throwing to force an abort. And I don't want
to re-throw PartialSuccessError since we're aborting the partial success.
Or should we allow PartialSuccessError on create? My thinking so far is
that PartialSuccessError only really makes sense when it's primarily a
group operation. In this case it's a single object operation with a
dependant group operation wrapped in a transaction.
>> + 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.
>> diff --git a/src/app/services/pool_service.rb b/src/app/services/pool_service.rb
>> new file mode 100644
>> index 0000000..8073067
>> --- /dev/null
>> +++ b/src/app/services/pool_service.rb
>> @@ -0,0 +1,60 @@
>>
>
> ...
>
>
>> + def svc_show(pool_id)
>> + # from before_filter
>> + @pool = Pool.find(pool_id)
>> + authorized!(Privilege::VIEW, at pool)
>> + end
>> +
>> + def update_perms
>> + set_perms(@pool)
>> + end
>>
>
> In the host controller, I addressed the same problem with
>
> private
> def lookup(id, priv)
> @pool = Pool.find(id)
> authorized!(priv, @pool)
> end
>
> Of course, I would prefer that here, too.
>
>
The reason for pulling out update_perms is to allow a different perm_obj
for vm pools on update. For VM pools update_perms calls
set_perms(@pool.parent)
>> + def svc_update(pool_id, pool_hash)
>> + # from before_filter
>> + @pool = Pool.find(params[:id])
>> + @parent = @pool.parent
>> + update_perms
>> + authorized!(Privilege::Modify)
>>
>
> Should be Privilege::MODIFY
>
>
Good catch here too.
> David
>
>
>
Scott
More information about the ovirt-devel
mailing list