[Ovirt-devel] [PATCH server] controller service layer refactoring for pools.
David Lutterkort
lutter at redhat.com
Wed Apr 29 19:22:03 UTC 2009
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.
> 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
> @@ -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.
> @@ -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.
> 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.
> @@ -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.
> + 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 ?
> 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
> @@ -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.
> 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.
> 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 ?
> 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 ?
> # 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.
> 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.
> + # 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 ?
> + 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.
> 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.
> + 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
More information about the ovirt-devel
mailing list