[Ovirt-devel] [PATCH server] controller service layer refactoring for pools.
Scott Seago
sseago at redhat.com
Mon May 4 18:01:07 UTC 2009
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.
>
>
Sending updated patch shortly with the fixes below:
>> diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb
>> index 2158e08..8261cd7 100644
>> --- a/src/app/controllers/hardware_controller.rb
>> +++ b/src/app/controllers/hardware_controller.rb
>>
>
>
>> @@ -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
>
>
Done
>> @@ -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.
>
>
Done
>> @@ -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 so we'll revisit this when we add the rescue_with stuff, but for now
I've actually changed this to take a symbol arg for the method name.
Since the only thing that's different here is which method is calling,
this avoids the if block with two method invocations (the args are the
same in both cases) -- I started to do blocks but I realized that I'd
have to repeat the method argument generation in each block, which is
also rather un-rubyish.
>> @@ -97,30 +108,111 @@ class PoolController < ApplicationController
>>
>> + 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 ?
>
fixed
>> 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
>
Fixed
>> @@ -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.
>
>
Fixed this -- and no alert clobbering on delete either.
>> 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.
>
Done
>> 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 ?
>
>
OK I'll pull this out into a later patch.
>> 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 ?
>
Fixed this
>> # 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.
>
>
I guess I'll un-do this for now then and later re-create this later, as
I don't trust git rebase -i to be useful for me to separate commits
_within_ a file.
>> 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_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
>
> David
>
>
>
Fixed
More information about the ovirt-devel
mailing list