[Ovirt-devel] [PATCH server 3/3] Additional refactoring of pool controllers (and subclasses)
Scott Seago
sseago at redhat.com
Fri May 8 19:34:11 UTC 2009
1) Moved all remaining auth/before_filter action into service layer
2) Added rdoc comments
3) Fixed a bug in 'add hosts/storage' functionality
4) Removed explicit error handling where it could be pushed up to the rescue_from handlers
Signed-off-by: Scott Seago <sseago at redhat.com>
---
src/app/controllers/hardware_controller.rb | 44 ++++++----------
src/app/controllers/pool_controller.rb | 50 ++++++-----------
src/app/controllers/resources_controller.rb | 52 ++++--------------
src/app/controllers/smart_pools_controller.rb | 24 ++-------
src/app/services/hardware_pool_service.rb | 61 ++++++++++++++++-----
src/app/services/pool_service.rb | 73 ++++++++++++++++++++-----
src/app/services/smart_pool_service.rb | 47 ++++++++++++----
src/app/services/vm_resource_pool_service.rb | 60 ++++++++++++++++-----
8 files changed, 237 insertions(+), 174 deletions(-)
diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb
index 7ef08db..56b6e08 100644
--- a/src/app/controllers/hardware_controller.rb
+++ b/src/app/controllers/hardware_controller.rb
@@ -28,8 +28,6 @@ class HardwareController < PoolController
verify :method => [:post, :delete], :only => :destroy,
:redirect_to => { :action => :list }
- before_filter :pre_modify, :only => [:move, :removestorage]
-
def index
if params[:path]
@pools = []
@@ -124,12 +122,12 @@ class HardwareController < PoolController
def hosts_json
if params[:exclude_host]
- pre_show_pool
+ svc_show(params[:id])
hosts = @pool.hosts
find_opts = {:conditions => ["id != ?", params[:exclude_host]]}
include_pool = false
elsif params[:id]
- pre_show_pool
+ svc_show(params[:id])
hosts = @pool.hosts
find_opts = {}
include_pool = false
@@ -146,6 +144,7 @@ class HardwareController < PoolController
end
def vm_pools_json
+ svc_show(params[:id])
json_list(Pool,
[:id, :name, :id],
[@pool, :children],
@@ -154,7 +153,7 @@ class HardwareController < PoolController
def storage_pools_json
if params[:id]
- pre_show_pool
+ svc_show(params[:id])
storage_pools = @pool.storage_pools
find_opts = {:conditions => "type != 'LvmStoragePool'"}
include_pool = false
@@ -171,11 +170,13 @@ class HardwareController < PoolController
end
def storage_volumes_json
+ svc_show(params[:id])
json_list(@pool.all_storage_volumes,
[:display_name, :size_in_gb, :get_type_label])
end
def move
+ svc_modify(params[:id])
@resource_type = params[:resource_type]
@id = params[:id]
@pools = HardwarePool.get_default_pool.full_set_nested(:method => :json_hash_element,
@@ -191,15 +192,17 @@ class HardwareController < PoolController
end
def additional_create_params
- {:resource_type => params[:resource_type],
- :resource_ids => params[:resource_ids],
- :parent_id => (params[:hardware_pool] ?
- params[:hardware_pool][:parent_id] :
- params[:parent_id])}
+ ret_hash = {:resource_ids => params[:resource_ids],
+ :parent_id => (params[:hardware_pool] ?
+ params[:hardware_pool][:parent_id] :
+ params[:parent_id])}
+ ret_hash[:resource_type] = Host if params[:resource_type] == "hosts"
+ ret_hash[:resource_type] = Storage if params[:resource_type] == "storage"
+ ret_hash
end
def add_hosts
- edit_items(@pool.id, :svc_move_hosts, :add)
+ edit_items(params[:id], :svc_move_hosts, :add)
end
def move_hosts
@@ -207,7 +210,7 @@ class HardwareController < PoolController
end
def add_storage
- edit_items(@pool.id, :svc_move_storage, :add)
+ edit_items(params[:id], :svc_move_storage, :add)
end
def move_storage
@@ -225,23 +228,8 @@ class HardwareController < PoolController
end
def removestorage
+ svc_modify(params[:id])
render :layout => 'popup'
end
-
- protected
- #filter methods
- def pre_new
- @pool = HardwarePool.new
- super
- end
- def pre_edit
- @pool = HardwarePool.find(params[:id])
- @parent = @pool.parent
- set_perms(@pool)
- end
- def pre_modify
- pre_edit
- authorize_admin
- end
end
diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb
index 6e41ac6..5c5949a 100644
--- a/src/app/controllers/pool_controller.rb
+++ b/src/app/controllers/pool_controller.rb
@@ -20,10 +20,6 @@
class PoolController < ApplicationController
- before_filter :pre_show_pool, :only => [:users_json, :show_tasks, :tasks,
- :vm_pools_json,
- :pools_json, :storage_volumes_json]
-
XML_OPTS = {
:include => [ :storage_pools, :hosts, :quota ]
}
@@ -36,6 +32,16 @@ class PoolController < ApplicationController
{}
end
+ def show_tasks
+ svc_show(params[:id])
+ super
+ end
+
+ def tasks
+ svc_show(params[:id])
+ super
+ end
+
def show
svc_show(params[:id])
render_show
@@ -65,6 +71,7 @@ class PoolController < ApplicationController
end
def users_json
+ svc_show(params[:id])
attr_list = []
attr_list << :grid_id if params[:checkboxes]
attr_list += [:uid, [:role, :name], :source]
@@ -97,6 +104,7 @@ class PoolController < ApplicationController
end
def new
+ svc_new(get_parent_id)
render :layout => 'popup'
end
@@ -141,44 +149,22 @@ class PoolController < ApplicationController
end
def edit
+ svc_modify(params[:id])
render :layout => 'popup'
end
def destroy
- alert = nil
- success = true
- status = :ok
- begin
- alert = svc_destroy(params[:id])
- rescue ActionError => error
- alert = error.message
- success = false
- status = :conflict
- rescue PermissionError => error
- alert = error.message
- success = false
- status = :forbidden
- rescue Exception => error
- alert = error.message
- success = false
- status = :method_not_allowed
- end
+ alert = svc_destroy(params[:id])
respond_to do |format|
- format.json { render :json => { :object => "pool", :success => success,
+ format.json { render :json => { :object => "pool", :success => true,
:alert => alert } }
- format.xml { head status }
+ format.xml { head(:ok) }
end
end
protected
- def pre_new
- @parent = Pool.find(params[:parent_id])
- set_perms(@parent)
- end
- def pre_show_pool
- @pool = Pool.find(params[:id])
- set_perms(@pool)
- authorize_view
+ def get_parent_id
+ params[:parent_id]
end
# FIXME: remove these when service transition is complete. these are here
# to keep from running permissions checks and other setup steps twice
diff --git a/src/app/controllers/resources_controller.rb b/src/app/controllers/resources_controller.rb
index edee59d..97d5ee9 100644
--- a/src/app/controllers/resources_controller.rb
+++ b/src/app/controllers/resources_controller.rb
@@ -68,7 +68,7 @@ class ResourcesController < PoolController
end
def vms_json
- pre_show_pool
+ svc_show(params[:id])
super(:full_items => @pool.vms, :find_opts => {}, :include_pool => :true)
end
@@ -78,8 +78,6 @@ class ResourcesController < PoolController
params[:parent_id])}
end
- #FIXME: we need permissions checks. user must have permission. We also need to fail
- # for pools that aren't currently empty
def delete
vm_pool_ids = params[:vm_pool_ids].split(",")
successes = []
@@ -94,48 +92,20 @@ class ResourcesController < PoolController
failures[@pool] = ex.message
end
end
- 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(', ')}."
+ unless failures.empty?
+ raise PartialSuccessError.new("Delete failed for some VM Pools",
+ failures, successes)
end
render :json => { :object => "vm_resource_pool", :success => success,
- :alert => alert }
+ :alert => "VM Pools were successfully deleted." }
end
def vm_actions
- begin
- alert = svc_vm_actions_hosts(params[:id], params[:vm_action],
- params[:vm_ids].split(","))
- @success_list = @vms
- @failures = {}
- render :layout => 'confirmation'
- rescue PermissionError
- raise
- rescue PartialSuccessError => error
- @success_list = error.successes
- @failures = error.failures
- render :layout => 'confirmation'
- rescue Exeption => ex
- flash[:errmsg] = 'Error queueing VM actions.'
- @success_list = []
- @failure_list = []
- end
+ @layout = 'confirmation'
+ alert = svc_vm_actions(params[:id], params[:vm_action],
+ params[:vm_ids].split(","))
+ @successes = @vms
+ @failures = {}
+ render :layout => @layout
end
-
- protected
- def pre_new
- @pool = VmResourcePool.new
- super
- end
- def pre_edit
- @pool = VmResourcePool.find(params[:id])
- @parent = @pool.parent
- @current_pool_id=@pool.id
- set_perms(@pool.parent)
- end
-
end
diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb
index 0198d47..83e72b5 100644
--- a/src/app/controllers/smart_pools_controller.rb
+++ b/src/app/controllers/smart_pools_controller.rb
@@ -21,7 +21,6 @@
class SmartPoolsController < PoolController
include SmartPoolService
- before_filter :pre_modify, :only => [:add_pool_dialog]
def show_vms
show
end
@@ -41,12 +40,11 @@ class SmartPoolsController < PoolController
end
def additional_create_params
- {:parent_id => (params[:hardware_pool] ?
- params[:hardware_pool][:parent_id] :
- params[:parent_id])}
+ {}
end
def add_pool_dialog
+ svc_modify(params[:id])
@selected_pools = @pool.tagged_pools.collect {|pool| pool.id}
render :layout => 'popup'
end
@@ -80,7 +78,7 @@ class SmartPoolsController < PoolController
def items_json_internal(item_class, item_assoc)
if params[:id]
- pre_show_pool
+ svc_show(params[:id])
full_items = @pool.send(item_assoc)
find_opts = {}
include_pool = false
@@ -152,20 +150,8 @@ class SmartPoolsController < PoolController
end
protected
- #filter methods
- def pre_new
- @pool = SmartPool.new
- @parent = DirectoryPool.get_or_create_user_root(get_login_user)
- set_perms(@parent)
- end
- def pre_edit
- @pool = SmartPool.find(params[:id])
- @parent = @pool.parent
- set_perms(@pool)
- end
- def pre_modify
- pre_edit
- authorize_admin
+ def get_parent_id
+ DirectoryPool.get_or_create_user_root(get_login_user).id
end
end
diff --git a/src/app/services/hardware_pool_service.rb b/src/app/services/hardware_pool_service.rb
index 51519fa..28040f3 100644
--- a/src/app/services/hardware_pool_service.rb
+++ b/src/app/services/hardware_pool_service.rb
@@ -22,43 +22,78 @@ module HardwarePoolService
include PoolService
+ # Load a new HardwarePool for creating
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] loads a new HardwarePool object into memory
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt> as specified by
+ # +parent_id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the parent pool
+ def svc_new(parent_id, attributes=nil)
+ @pool = HardwarePool.new(attributes)
+ super(parent_id)
+ end
+
+ # Save a new HardwarePool
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the newly saved HardwarePool
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt> as specified by
+ # +parent_id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the parent pool
def svc_create(pool_hash, other_args)
- # from before_filter
- @pool = HardwarePool.new(pool_hash)
- @parent = Pool.find(other_args[:parent_id])
- authorized!(Privilege::MODIFY, at parent)
+ svc_new(other_args[:parent_id], pool_hash)
- 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)
+ if other_args[:resource_type]
+ svc_move_items_internal(@pool.id, other_args[:resource_type],
+ other_args[:resource_ids].split(","),
+ @pool.id)
end
- # wrapped in a transaction, so fail on partial success
+ # wrapped in a transaction, so fail on partial success
rescue PartialSuccessError => ex
# Raising ActionError here since we're aborting the transaction. Errors
# on creation here result in no persistent changes to the database.
raise ActionError.new("Could not move all hosts or storage to this pool")
end
end
- return alert
+ return "Hardware Pool was successfully created."
end
+ # Move Hosts identified by +host_ids+ from one Hardware Pool to another. The
+ # target pool for the hosts is specified as +target_pool_id+.
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the current HardwarePool
+ # [<tt>@parent</tt>] the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the target pool as well as the current
+ # pool for each moved host
def svc_move_hosts(pool_id, host_ids, target_pool_id)
svc_move_items_internal(pool_id, Host, host_ids, target_pool_id)
end
+
+ # Move Storage Pools identified by +storage_pool_ids+ from one Hardware Pool
+ # to another. The target pool for the hosts is specified as +target_pool_id+.
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the current HardwarePool
+ # [<tt>@parent</tt>] the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the target pool as well as the current
+ # pool for each moved Storage Pool
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
+ lookup(pool_id, Privilege::MODIFY)
resources = item_class.find(resource_ids)
diff --git a/src/app/services/pool_service.rb b/src/app/services/pool_service.rb
index 2377094..c05b0dc 100644
--- a/src/app/services/pool_service.rb
+++ b/src/app/services/pool_service.rb
@@ -22,34 +22,69 @@ module PoolService
include ApplicationService
- def svc_show(pool_id)
- # from before_filter
- @pool = Pool.find(pool_id)
- authorized!(Privilege::VIEW, at pool)
+ # Load the Pool with +id+ for viewing
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] stores the Pool with +id+
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::VIEW</tt>] for the pool
+ def svc_show(id)
+ lookup(id, Privilege::VIEW)
end
- def update_perms
- set_perms(@pool)
+ # Load the Pool with +id+ for editing
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] stores the Pool with +id+
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the pool
+ def svc_modify(id)
+ lookup(id, Privilege::MODIFY)
end
+
def additional_update_actions(pool, pool_hash)
end
- def svc_update(pool_id, pool_hash)
+ # Update attributes for the Pool with +id+
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] stores the Pool with +id+
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the pool
+ def svc_update(id, pool_hash)
# from before_filter
- @pool = Pool.find(params[:id])
- @parent = @pool.parent
- update_perms
- authorized!(Privilege::MODIFY)
+ svc_modify(id)
Pool.transaction do
additional_update_actions(@pool, pool_hash)
@pool.update_attributes!(pool_hash)
end
end
- def svc_destroy(pool_id)
- # from before_filter
- @pool = Pool.find(pool_id)
- authorized!(Privilege::MODIFY, @pool)
+ # Load a new Pool for creating
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] loads a new Pool object into memory
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt> as specified by
+ # +parent_id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the parent pool
+ def svc_new(parent_id, attributes=nil)
+ @parent = Pool.find(parent_id)
+ authorized!(Privilege::MODIFY, @parent)
+ end
+
+ # Destroy the Pool object with +id+
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the destroyed pool
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the pool
+ def svc_destroy(id)
+ svc_modify(id)
check_destroy_preconditions
@pool.destroy
return "Pool was successfully deleted."
@@ -57,4 +92,12 @@ module PoolService
def check_destroy_preconditions
end
+
+ protected
+ def lookup(id, priv, use_parent_perms=false)
+ @pool = Pool.find(id)
+ @parent = @pool.parent
+ @current_pool_id = @pool.id if use_parent_perms
+ authorized!(priv, use_parent_perms ? @parent : @pool)
+ end
end
diff --git a/src/app/services/smart_pool_service.rb b/src/app/services/smart_pool_service.rb
index 344b9e8..67adfe0 100644
--- a/src/app/services/smart_pool_service.rb
+++ b/src/app/services/smart_pool_service.rb
@@ -22,23 +22,46 @@ module SmartPoolService
include PoolService
- def svc_create(pool_hash, other_args)
- # from before_filter
- @pool = SmartPool.new(pool_hash)
- @parent = DirectoryPool.get_or_create_user_root(get_login_user)
- authorized!(Privilege::MODIFY, at parent)
+ # Load a new SmartPool for creating
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] loads a new SmartPool object into memory
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt> as specified by
+ # +parent_id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the pool
+ def svc_new(parent_id, attributes=nil)
+ @pool = SmartPool.new(attributes)
+ super(parent_id)
+ end
- alert = "Smart Pool was successfully created."
+ # Save a new SmartPool
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the newly saved SmartPool
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt> as specified by
+ # +parent_id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the pool
+ def svc_create(pool_hash, other_args)
+ svc_new(nil, pool_hash)
@pool.create_with_parent(@parent)
- return alert
+ return "Smart Pool was successfully created."
end
- # if item_class is nil, resource_ids is an array of [class, id] pairs
+ # Add or remove (depending on +item_action+) objects represneted by
+ # +resource_ids+ in the smart pool identified by +pool_id+. Item type
+ # is identified by +item_class+. If +item_class+ is nil, then
+ # +resource_ids+ is an array of [class, id] pairs.
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the current SmartPool
+ # [<tt>@parent</tt>] the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the current smart pool
+ # [<tt>Privilege::VIEW</tt>] for any item being added to this smart pool
def svc_add_remove_items(pool_id, item_class, item_action, resource_ids)
- # from before_filter
- @pool = SmartPool.find(pool_id)
- @parent = @pool.parent
- authorized!(Privilege::MODIFY, at pool)
+ svc_modify(pool_id)
unless [:add, :remove].include?(item_action)
raise ActionError.new("Invalid action #{item_action}")
end
diff --git a/src/app/services/vm_resource_pool_service.rb b/src/app/services/vm_resource_pool_service.rb
index 30f7106..00cf982 100644
--- a/src/app/services/vm_resource_pool_service.rb
+++ b/src/app/services/vm_resource_pool_service.rb
@@ -22,22 +22,56 @@ module VmResourcePoolService
include PoolService
- def svc_create(pool_hash, other_args)
- # from before_filter
- @pool = VmResourcePool.new(pool_hash)
- @parent = Pool.find(other_args[:parent_id])
- authorized!(Privilege::MODIFY, at parent)
+ # Load the VmResourcePool with +id+ for editing
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] stores the Pool with +id+
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt>
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the parent pool
+ def svc_modify(id)
+ lookup(id, Privilege::MODIFY, true)
+ end
- alert = "VM Pool was successfully created."
- @pool.create_with_parent(@parent)
- return alert
+ # Load a new VmResourcePool for creating
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] loads a new VmResourcePool object into memory
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt> as specified by
+ # +parent_id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the parent pool
+ def svc_new(parent_id, attributes=nil)
+ @pool = VmResourcePool.new(attributes)
+ super(parent_id)
end
- def update_perms
- @current_pool_id=@pool.id
- set_perms(@pool.parent)
+ # Save a new VmResourcePool
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the newly saved VmResourcePool
+ # [<tt>@parent</tt>] stores the parent of <tt>@pool</tt> as specified by
+ # +parent_id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the parent pool
+ def svc_create(pool_hash, other_args)
+ svc_new(other_args[:parent_id], pool_hash)
+ @pool.create_with_parent(@parent)
+ return "VM Pool was successfully created."
end
+ # Perform action +vm_action+ on vms identified by +vm_id+ within Pool
+ # +pool_id+
+ #
+ # === Instance variables
+ # [<tt>@pool</tt>] the current VmResourcePool
+ # [<tt>@parent</tt>] the parent of <tt>@pool</tt>
+ # [<tt>@action</tt>] action identified by +vm_action+
+ # [<tt>@action_label</tt>] label for action identified by +vm_action+
+ # [<tt>@vms</tt>] VMs identified by +vm_ids+
+ # === Required permissions
+ # permission is action-specific as determined by
+ # <tt>VmTask.action_privilege(@action)</tt>
def svc_vm_actions(pool_id, vm_action, vm_ids)
# from before_filter
@pool = VmResourcePool.find(pool_id)
@@ -65,12 +99,10 @@ module VmResourcePoolService
end
end
unless failed_vms.empty?
- raise PartialSuccessError.new("#{@action} only partially successful",
+ raise PartialSuccessError.new("#{@action} failed for some VMs",
failed_vms, successful_vms)
end
return "Action #{@action} successful."
end
-
-
end
--
1.6.0.6
More information about the ovirt-devel
mailing list