Scott Seago
2009-Apr-20 20:26 UTC
[Ovirt-devel] [PATCH server] Revised attempt at refactoring the vm_controller using the service layer.
This code is based on David Lutterkort's proposal outlined in https://www.redhat.com/archives/ovirt-devel/2009-April/msg00086.html I've included service-layer methods for show, create, update, destroy, vm_action, and cancel_queued_tasks. These are the concrete actions that make sense as part of the API. Most of the remaining vm_controller methods are pure-WUI actions -- for things such as generating the web form for a new VM (the subsequent 'create' action _is_ part of the API). There's a certain amount of temporary code that we need in place while only some of the controllers have been converted, as we still need permissions pre-filters for non-converted create, etc. methods -- but for the converted controllers we don't want to perform actions in before_filters that will be done as part of the services layer. Any temporary methods, etc. that will go away when the last controller is converted have been marked clearly with FIXME comments. Revised version passes perm_obj arg into set_perms -- and always uses this method to set @perm_obj rather than setting it directly. I've also cleaned the permission-setting calls in general, removing more of the no-longer-valid redirects, etc. Signed-off-by: Scott Seago <sseago at redhat.com> --- src/app/controllers/application.rb | 113 ++++++---- src/app/controllers/hardware_controller.rb | 8 +- src/app/controllers/host_controller.rb | 23 +-- src/app/controllers/network_controller.rb | 2 +- src/app/controllers/nic_controller.rb | 8 +- src/app/controllers/permission_controller.rb | 22 +-- src/app/controllers/pool_controller.rb | 16 +- src/app/controllers/quota_controller.rb | 16 +- src/app/controllers/resources_controller.rb | 4 +- src/app/controllers/smart_pools_controller.rb | 6 +- src/app/controllers/storage_controller.rb | 21 +-- src/app/controllers/storage_volume_controller.rb | 24 +-- src/app/controllers/task_controller.rb | 5 +- src/app/controllers/vm_controller.rb | 241 +++++----------------- src/app/models/vm.rb | 1 + src/app/services/application_service.rb | 57 +++++ src/app/services/vm_service.rb | 183 ++++++++++++++++ src/app/views/vm/_form.rhtml | 3 +- 18 files changed, 409 insertions(+), 344 deletions(-) create mode 100644 src/app/services/application_service.rb create mode 100644 src/app/services/vm_service.rb diff --git a/src/app/controllers/application.rb b/src/app/controllers/application.rb index fa88773..e5f4d4b 100644 --- a/src/app/controllers/application.rb +++ b/src/app/controllers/application.rb @@ -20,19 +20,35 @@ # Filters added to this controller apply to all controllers in the application. # Likewise, all the methods added will be available for all controllers. +# FIXME: once all controller classes include this, remove here +require 'services/application_service' + class ApplicationController < ActionController::Base + # FIXME: once all controller classes include this, remove here + include ApplicationService + # Pick a unique cookie name to distinguish our session data from others' session :session_key => '_ovirt_session_id' init_gettext "ovirt" layout :choose_layout + # FIXME: once service layer is complete, the following before_filters will be + # removed as their functionality has been moved to the service layer + # pre_create + # pre_edit will remain only for :edit, not :update or :destroy + # pre_show + # authorize_admin will remain only for :new, :edit before_filter :pre_new, :only => [:new] before_filter :pre_create, :only => [:create] - before_filter :pre_edit, :only => [:edit, :update, :destroy] + before_filter :pre_edit, :only => [:edit] + # the following is to facilitate transition to service layer + before_filter :tmp_pre_update, :only => [:update, :destroy] before_filter :pre_show, :only => [:show] - before_filter :authorize_admin, :only => [:new, :create, :edit, :update, :destroy] + before_filter :authorize_admin, :only => [:new, :edit] + before_filter :tmp_authorize_admin, :only => [:create, :update, :destroy] before_filter :is_logged_in, :get_help_section + def choose_layout if(params[:component_layout]) return (ENV["RAILS_ENV"] != "production")?'components/' << params[:component_layout]:'redux' @@ -61,62 +77,66 @@ class ApplicationController < ActionController::Base (ENV["RAILS_ENV"] == "production") ? session[:user] : "ovirtadmin" end - def set_perms(hwpool) - @user = get_login_user - @can_view = hwpool.can_view(@user) - @can_control_vms = hwpool.can_control_vms(@user) - @can_modify = hwpool.can_modify(@user) - @can_view_perms = hwpool.can_view_perms(@user) - @can_set_perms = hwpool.can_set_perms(@user) - end - protected # permissions checking def pre_new end - def pre_create - end def pre_edit end + + # FIXME: remove these when service layer transition is complete + def tmp_pre_update + pre_edit + end + def tmp_authorize_admin + authorize_admin + end + def pre_create + end def pre_show end + def authorize_view(msg=nil) + authorize_action(Privilege::VIEW,msg) + end def authorize_user(msg=nil) - authorize_action(false,msg) + authorize_action(Privilege::VM_CONTROL,msg) end def authorize_admin(msg=nil) - authorize_action(true,msg) - end - def authorize_action(is_modify_action, msg=nil) - msg ||= 'You do not have permission to create or modify this item ' - if @perm_obj - set_perms(@perm_obj) - unless (is_modify_action ? @can_modify : @can_control_vms) - respond_to do |format| - format.html do - @title = "Access denied" - @errmsg = msg - @ajax = params[:ajax] - @nolayout = params[:nolayout] - if @ajax - render :template => 'layouts/popup-error', :layout => 'tabs-and-content' - elsif @nolayout - render :template => 'layouts/popup-error', :layout => 'help-and-content' - else - render :template => 'layouts/popup-error', :layout => 'popup' - end - end - format.json do - @json_hash ||= {} - @json_hash[:success] = false - @json_hash[:alert] = msg - render :json => @json_hash - end - format.xml { head :forbidden } + authorize_action(Privilege::MODIFY,msg) + end + def authorize_action(privilege, msg=nil) + msg ||= 'You have insufficient privileges to perform action.' + unless authorized?(privilege) + handle_auth_error(msg) + false + else + true + end + end + def handle_auth_error(msg) + respond_to do |format| + format.html do + @title = "Access denied" + @errmsg = msg + @ajax = params[:ajax] + @nolayout = params[:nolayout] + if @ajax + render :template => 'layouts/popup-error', :layout => 'tabs-and-content' + elsif @nolayout + render :template => 'layouts/popup-error', :layout => 'help-and-content' + else + render :template => 'layouts/popup-error', :layout => 'popup' end - false end + format.json do + @json_hash ||= {} + @json_hash[:success] = false + @json_hash[:alert] = msg + render :json => @json_hash + end + format.xml { head :forbidden } end end @@ -155,6 +175,11 @@ class ApplicationController < ActionController::Base render :json => json_hash(full_items, attributes, arg_list, find_opts, id_method).to_json end - + def json_error(obj_type, obj, exception) + json_hash = { :object => obj_type, :success => false} + json_hash[:errors] = obj.errors.localize_error_messages.to_a if obj + json_hash[:alert] = exception.message if obj.errors.size == 0 + render :json => json_hash + end end diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb index 726e5bd..094eef0 100644 --- a/src/app/controllers/hardware_controller.rb +++ b/src/app/controllers/hardware_controller.rb @@ -71,11 +71,7 @@ class HardwareController < PoolController if id @pool = Pool.find(id) set_perms(@pool) - unless @pool.has_privilege(@user, privilege) - flash[:notice] = 'You do not have permission to access this hardware pool: redirecting to top level' - redirect_to :controller => "dashboard" - return - end + return unless authorize_action(privilege) end if @pool pools = @pool.children @@ -386,7 +382,7 @@ class HardwareController < PoolController def pre_edit @pool = HardwarePool.find(params[:id]) @parent = @pool.parent - @perm_obj = @pool + set_perms(@pool) @current_pool_id=@pool.id end def pre_show diff --git a/src/app/controllers/host_controller.rb b/src/app/controllers/host_controller.rb index f0b8c2b..7f808dc 100644 --- a/src/app/controllers/host_controller.rb +++ b/src/app/controllers/host_controller.rb @@ -53,12 +53,7 @@ class HostController < ApplicationController def show - set_perms(@perm_obj) - unless @can_view - flash[:notice] = 'You do not have permission to view this host: redirecting to top level' - #perm errors for ajax should be done differently - redirect_to :controller => 'dashboard', :action => 'list' - else + if authorize_view respond_to do |format| format.html { render :layout => 'selection' } format.xml { render :xml => @host.to_xml(:include => [ :cpus ] ) } @@ -68,13 +63,9 @@ class HostController < ApplicationController def quick_summary pre_show - set_perms(@perm_obj) - unless @can_view - flash[:notice] = 'You do not have permission to view this host: redirecting to top level' - #perm errors for ajax should be done differently - redirect_to :controller => 'dashboard', :action => 'list' + if authorize_view + render :layout => false end - render :layout => false end # retrieves data used by snapshot graphs @@ -89,7 +80,7 @@ class HostController < ApplicationController def pre_addhost @pool = Pool.find(params[:hardware_pool_id]) @parent = @pool.parent - @perm_obj = @pool + set_perms(@pool) @current_pool_id=@pool.id authorize_admin end @@ -192,14 +183,14 @@ class HostController < ApplicationController end def pre_action @host = Host.find(params[:id]) - @perm_obj = @host.hardware_pool + set_perms(@host.hardware_pool) @json_hash = { :object => :host } authorize_admin end def pre_show @host = Host.find(params[:id]) - @perm_obj = @host.hardware_pool - @current_pool_id=@perm_obj.id + set_perms(@host.hardware_pool) + @current_pool_id=@host.hardware_pool.id end diff --git a/src/app/controllers/network_controller.rb b/src/app/controllers/network_controller.rb index a99cf8d..244d041 100644 --- a/src/app/controllers/network_controller.rb +++ b/src/app/controllers/network_controller.rb @@ -28,7 +28,7 @@ class NetworkController < ApplicationController # or by extending permission model to accomodate # any object @default_pool = HardwarePool.get_default_pool - @perm_obj=@default_pool + set_perms(@default_pool) super('You do not have permission to access networks') end diff --git a/src/app/controllers/nic_controller.rb b/src/app/controllers/nic_controller.rb index 85d4315..fdbda06 100644 --- a/src/app/controllers/nic_controller.rb +++ b/src/app/controllers/nic_controller.rb @@ -23,11 +23,7 @@ class NicController < ApplicationController :redirect_to => { :controller => 'dashboard' } def show - set_perms(@perm_obj) - unless @can_view - flash[:notice] = 'You do not have permission to view this NIC: redirecting to top level' - redirect_to :controller => 'pool', :action => 'list' - end + authorize_view end def new @@ -62,7 +58,7 @@ class NicController < ApplicationController end def pre_show @nic = Nic.find(params[:id]) - @perm_obj = @nic.host.hardware_pool + set_perms(@nic.host.hardware_pool) end end diff --git a/src/app/controllers/permission_controller.rb b/src/app/controllers/permission_controller.rb index 3bfbffa..d4c3fb5 100644 --- a/src/app/controllers/permission_controller.rb +++ b/src/app/controllers/permission_controller.rb @@ -29,11 +29,7 @@ class PermissionController < ApplicationController def show @permission = Permission.find(params[:id]) set_perms(@permission.pool) - # admin permission required to view permissions - unless @can_view_perms - flash[:notice] = 'You do not have permission to view this permission record' - redirect_to_parent - end + authorize_action(Privilege::PERM_VIEW) end def new @@ -45,10 +41,7 @@ class PermissionController < ApplicationController @roles = Role.find(:all).collect{ |role| [role.name, role.id] } set_perms(@permission.pool) # admin permission required to view permissions - unless @can_set_perms - flash[:notice] = 'You do not have permission to create this permission record' - redirect_to_parent - else + if authorize_action(Privilege::PERM_SET) render :layout => 'popup' end end @@ -56,11 +49,7 @@ class PermissionController < ApplicationController def create @permission = Permission.new(params[:permission]) set_perms(@permission.pool) - unless @can_set_perms - # FIXME: need to handle proper error messages w/ ajax - flash[:notice] = 'You do not have permission to create this permission record' - redirect_to_parent - else + if authorize_action(Privilege::PERM_SET) begin @permission.save_with_new_children render :json => { :object => "permission", :success => true, @@ -118,10 +107,7 @@ class PermissionController < ApplicationController def destroy @permission = Permission.find(params[:id]) set_perms(@permission.pool) - unless @can_set_perms - flash[:notice] = 'You do not have permission to delete this permission record' - redirect_to_parent - else + if authorize_action(Privilege::PERM_SET) pool = @permission.pool if @permission.destroy if pool diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb index 22cf1a4..4d83bbe 100644 --- a/src/app/controllers/pool_controller.rb +++ b/src/app/controllers/pool_controller.rb @@ -104,7 +104,7 @@ class PoolController < ApplicationController protected def pre_new @parent = Pool.find(params[:parent_id]) - @perm_obj = @parent + set_perms(@parent) @current_pool_id=@parent.id end def pre_create @@ -114,24 +114,16 @@ class PoolController < ApplicationController else @parent = Pool.find(params[:parent_id]) end - @perm_obj = @parent + set_perms(@parent) @current_pool_id=@parent.id end def pre_show_pool pre_show end def pre_show - @perm_obj = @pool + set_perms(@pool) @current_pool_id=@pool.id - set_perms(@perm_obj) - unless @can_view - flash[:notice] = 'You do not have permission to view this pool: redirecting to top level' - respond_to do |format| - format.html { redirect_to :controller => "dashboard" } - format.xml { head :forbidden } - end - return - end + authorize_view end end diff --git a/src/app/controllers/quota_controller.rb b/src/app/controllers/quota_controller.rb index 17fdc20..d4fbcd0 100644 --- a/src/app/controllers/quota_controller.rb +++ b/src/app/controllers/quota_controller.rb @@ -28,13 +28,7 @@ class QuotaController < ApplicationController def show @quota = Quota.find(params[:id]) - set_perms(@quota.pool) - - unless @can_view - flash[:notice] = 'You do not have permission to view this quota: redirecting to top level' - redirect_to_parent - end - + authorize_view end def new @@ -83,19 +77,19 @@ class QuotaController < ApplicationController protected def pre_new @quota = Quota.new( { :pool_id => params[:pool_id]}) - @perm_obj = @quota.pool + set_perms(@quota.pool) end def pre_create @quota = Quota.new(params[:quota]) - @perm_obj = @quota.pool + set_perms(@quota.pool) end def pre_show @quota = Quota.find(params[:id]) - @perm_obj = @quota + set_perms(@quota.pool) end def pre_edit @quota = Quota.find(params[:id]) - @perm_obj = @quota.pool + set_perms(@quota.pool) end end diff --git a/src/app/controllers/resources_controller.rb b/src/app/controllers/resources_controller.rb index b57fae6..6b61439 100644 --- a/src/app/controllers/resources_controller.rb +++ b/src/app/controllers/resources_controller.rb @@ -166,7 +166,7 @@ class ResourcesController < PoolController def pre_edit @pool = VmResourcePool.find(params[:id]) @parent = @pool.parent - @perm_obj = @pool.parent + set_perms(@pool.parent) @current_pool_id=@pool.id end def pre_show @@ -177,7 +177,7 @@ class ResourcesController < PoolController def pre_vm_actions @pool = VmResourcePool.find(params[:id]) @parent = @pool.parent - @perm_obj = @pool + set_perms(@pool) authorize_user end diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb index f5f3c9d..93c81ee 100644 --- a/src/app/controllers/smart_pools_controller.rb +++ b/src/app/controllers/smart_pools_controller.rb @@ -199,19 +199,19 @@ class SmartPoolsController < PoolController def pre_new @pool = SmartPool.new @parent = DirectoryPool.get_or_create_user_root(get_login_user) - @perm_obj = @parent + set_perms(@parent) @current_pool_id=@parent.id end def pre_create @pool = SmartPool.new(params[:smart_pool]) @parent = DirectoryPool.get_or_create_user_root(get_login_user) - @perm_obj = @parent + set_perms(@parent) @current_pool_id=@parent.id end def pre_edit @pool = SmartPool.find(params[:id]) @parent = @pool.parent - @perm_obj = @pool + set_perms(@pool) @current_pool_id=@pool.id end def pre_show diff --git a/src/app/controllers/storage_controller.rb b/src/app/controllers/storage_controller.rb index 98ba0f6..2cb5464 100644 --- a/src/app/controllers/storage_controller.rb +++ b/src/app/controllers/storage_controller.rb @@ -47,10 +47,7 @@ class StorageController < ApplicationController if @attach_to_pool pool = HardwarePool.find(@attach_to_pool) set_perms(pool) - unless @can_view - flash[:notice] = 'You do not have permission to view this storage pool list: redirecting to top level' - redirect_to :controller => 'dashboard' - else + if authorize_view conditions = "hardware_pool_id is null" conditions += " or hardware_pool_id=#{pool.parent_id}" if pool.parent @storage_pools = StoragePool.find(:all, :conditions => conditions) @@ -71,13 +68,7 @@ class StorageController < ApplicationController def show @storage_pool = StoragePool.find(params[:id]) set_perms(@storage_pool.hardware_pool) - unless @can_view - flash[:notice] = 'You do not have permission to view this storage pool: redirecting to top level' - respond_to do |format| - format.html { redirect_to :controller => 'dashboard' } - format.xml { head :forbidden } - end - else + if authorize_view respond_to do |format| format.html { render :layout => 'selection' } format.xml { @@ -234,7 +225,7 @@ class StorageController < ApplicationController def pre_new @hardware_pool = HardwarePool.find(params[:hardware_pool_id]) - @perm_obj = @hardware_pool + set_perms(@hardware_pool) authorize_admin @storage_pools = @hardware_pool.storage_volumes @storage_types = StoragePool::STORAGE_TYPE_PICKLIST @@ -250,7 +241,7 @@ class StorageController < ApplicationController new_params[:port] = 3260 end @storage_pool = StoragePool.factory(params[:storage_type], new_params) - @perm_obj = @storage_pool.hardware_pool + set_perms(@storage_pool.hardware_pool) authorize_admin end def pre_create @@ -259,11 +250,11 @@ class StorageController < ApplicationController type = pool.delete(:storage_type) end @storage_pool = StoragePool.factory(type, pool) - @perm_obj = @storage_pool.hardware_pool + set_perms(@storage_pool.hardware_pool) end def pre_edit @storage_pool = StoragePool.find(params[:id]) - @perm_obj = @storage_pool.hardware_pool + set_perms(@storage_pool.hardware_pool) end def pre_pool_admin pre_edit diff --git a/src/app/controllers/storage_volume_controller.rb b/src/app/controllers/storage_volume_controller.rb index 9b0b9e0..b6b0593 100644 --- a/src/app/controllers/storage_volume_controller.rb +++ b/src/app/controllers/storage_volume_controller.rb @@ -94,19 +94,12 @@ class StorageVolumeController < ApplicationController @storage_volume = StorageVolume.find(params[:id]) set_perms(@storage_volume.storage_pool.hardware_pool) @storage_pool = @storage_volume.storage_pool - unless @can_view - flash[:notice] = 'You do not have permission to view this storage volume: redirecting to top level' - respond_to do |format| - format.html { redirect_to :controller => 'dashboard' } - format.json { redirect_to :controller => 'dashboard' } - format.xml { head :forbidden } - end - else + if authorize_view respond_to do |format| format.html { render :layout => 'selection' } format.json do attr_list = [] - attr_list << :id if (@storage_pool.user_subdividable and @can_modify) + attr_list << :id if (@storage_pool.user_subdividable and authorized?(Privilege::MODIFY) attr_list += [:display_name, :size_in_gb, :get_type_label] json_list(@storage_pool.storage_volumes, attr_list) end @@ -118,13 +111,8 @@ class StorageVolumeController < ApplicationController def destroy @storage_volume = StorageVolume.find(params[:id]) set_perms(@storage_volume.storage_pool.hardware_pool) - unless @can_modify and @storage_volume.storage_pool.user_subdividable - respond_to do |format| - format.json { render :json => { :object => "storage_volume", - :success => false, - :alert => "You do not have permission to delete this storage volume." } } - format.xml { head :forbidden } - end + unless authorized?(Privilege::MODIFY) and @storage_volume.storage_pool.user_subdividable + handle_auth_error("You do not have permission to delete this storage volume.") else alert, success = delete_volume_internal(@storage_volume) respond_to do |format| @@ -141,14 +129,14 @@ class StorageVolumeController < ApplicationController type = volume.delete(:storage_type) end @storage_volume = StorageVolume.factory(type, volume) - @perm_obj = @storage_volume.storage_pool.hardware_pool + set_perms(@storage_volume.storage_pool.hardware_pool) authorize_admin end private def new_volume_internal(storage_pool, new_params) @storage_volume = StorageVolume.factory(storage_pool.get_type_label, new_params) - @perm_obj = @storage_volume.storage_pool.hardware_pool + set_perms(@storage_volume.storage_pool.hardware_pool) authorize_admin end diff --git a/src/app/controllers/task_controller.rb b/src/app/controllers/task_controller.rb index 647d69c..9756f79 100644 --- a/src/app/controllers/task_controller.rb +++ b/src/app/controllers/task_controller.rb @@ -29,10 +29,7 @@ class TaskController < ApplicationController elsif @task[:type] == HostTask.name set_perms(@task.host.hardware_pool) end - unless @can_view - flash[:notice] = 'You do not have permission to view this task: redirecting to top level' - redirect_to :controller => 'dashboard' - end + authorize_view end diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb index a81f6b5..e3dc9ef 100644 --- a/src/app/controllers/vm_controller.rb +++ b/src/app/controllers/vm_controller.rb @@ -17,13 +17,16 @@ # MA 02110-1301, USA. A copy of the GNU General Public License is # also available at http://www.gnu.org/copyleft/gpl.html. require 'socket' +require 'services/vm_service' class VmController < ApplicationController + include VmService + # GETs should be safe (see http://www.w3.org/2001/tag/doc/whenToUseGet.html) verify :method => :post, :only => [ :destroy, :create, :update ], :redirect_to => { :controller => 'dashboard' } - before_filter :pre_vm_action, :only => [:vm_action, :cancel_queued_tasks, :console] + before_filter :pre_console, :only => [:console] def index @vms = Vm.find(:all, @@ -38,13 +41,13 @@ class VmController < ApplicationController end def show - set_perms(@perm_obj) - @actions = @vm.get_action_hash(@user) - unless @can_view - flash[:notice] = 'You do not have permission to view this vm: redirecting to top level' - redirect_to :controller => 'resources', :controller => 'dashboard' + begin + svc_show(params[:id]) + @actions = @vm.get_action_hash(@user) + render :layout => 'selection' + rescue PermissionError => perm_error + handle_auth_error(perm_error.message) end - render :layout => 'selection' end def add_to_smart_pool @@ -58,38 +61,15 @@ class VmController < ApplicationController end def create + params[:vm][:forward_vnc] = params[:forward_vnc] begin - Vm.transaction do - @vm.save! - _setup_vm_provision(params) - @task = VmTask.new({ :user => @user, - :task_target => @vm, - :action => VmTask::ACTION_CREATE_VM, - :state => Task::STATE_QUEUED}) - @task.save! - end - start_now = params[:start_now] - if (start_now) - if @vm.get_action_list.include?(VmTask::ACTION_START_VM) - @task = VmTask.new({ :user => @user, - :task_target => @vm, - :action => VmTask::ACTION_START_VM, - :state => Task::STATE_QUEUED}) - @task.save! - alert = "VM was successfully created. VM Start action queued." - else - alert = "VM was successfully created. Resources are not available to start VM now." - end - else - alert = "VM was successfully created." - end + alert = svc_create(params[:vm], params[:start_now]) render :json => { :object => "vm", :success => true, :alert => alert } + rescue PermissionError => perm_error + handle_auth_error(perm_error.message) rescue Exception => error - # FIXME: need to distinguish vm vs. task save errors (but should mostly be vm) - render :json => { :object => "vm", :success => false, - :errors => @vm.errors.localize_error_messages.to_a } + json_error("vm", @vm, error) end - end def edit @@ -98,58 +78,20 @@ class VmController < ApplicationController end def update + params[:vm][:forward_vnc] = params[:forward_vnc] begin - #needs restart if certain fields are changed (since those will only take effect the next startup) - needs_restart = false - unless @vm.get_pending_state == Vm::STATE_STOPPED - Vm::NEEDS_RESTART_FIELDS.each do |field| - unless @vm[field].to_s == params[:vm][field] - needs_restart = true - break - end - end - current_storage_ids = @vm.storage_volume_ids.sort - new_storage_ids = params[:vm][:storage_volume_ids] - new_storage_ids = [] unless new_storage_ids - new_storage_ids = new_storage_ids.sort.collect {|x| x.to_i } - needs_restart = true unless current_storage_ids == new_storage_ids - end - - params[:vm][:forward_vnc] = params[:forward_vnc] - params[:vm][:needs_restart] = 1 if needs_restart - @vm.update_attributes!(params[:vm]) - _setup_vm_provision(params) - - if (params[:start_now] and @vm.get_action_list.include?(VmTask::ACTION_START_VM) ) - @task = VmTask.new({ :user => @user, - :task_target => @vm, - :action => VmTask::ACTION_START_VM, - :state => Task::STATE_QUEUED}) - @task.save! - elsif ( params[:restart_now] and @vm.get_action_list.include?(VmTask::ACTION_SHUTDOWN_VM) ) - @task = VmTask.new({ :user => @user, - :task_target => @vm, - :action => VmTask::ACTION_SHUTDOWN_VM, - :state => Task::STATE_QUEUED}) - @task.save! - @task = VmTask.new({ :user => @user, - :task_target => @vm, - :action => VmTask::ACTION_START_VM, - :state => Task::STATE_QUEUED}) - @task.save! - end - - - render :json => { :object => "vm", :success => true, - :alert => 'Vm was successfully updated.' } - rescue + alert = svc_update(params[:id], params[:vm], params[:start_now], + params[:restart_now]) + render :json => { :object => "vm", :success => true, :alert => alert } + rescue Exception => error # FIXME: need to distinguish vm vs. task save errors (but should mostly be vm) - render :json => { :object => "vm", :success => false, - :errors => @vm.errors.localize_error_messages.to_a } + json_error("vm", @vm, error) end end #FIXME: we need permissions checks. user must have permission. Also state checks + # this should probably be implemented as an action on the containing VM pool once + # that service module is defined def delete vm_ids_str = params[:vm_ids] vm_ids = vm_ids_str.split(",").collect {|x| x.to_i} @@ -181,15 +123,13 @@ class VmController < ApplicationController end def destroy - vm_resource_pool = @vm.vm_resource_pool_id - if (@vm.is_destroyable?) - destroy_cobbler_system(@vm) - @vm.destroy - render :json => { :object => "vm", :success => true, - :alert => "Virtual Machine was successfully deleted." } - else - render :json => { :object => "vm", :success => false, - :alert => "Vm must be stopped to delete it." } + begin + alert = svc_destroy(params[:id]) + render :json => { :object => "vm", :success => true, :alert => alert } + rescue ActionError => error + json_error("vm", @vm, error) + rescue Exception => error + json_error("vm", @vm, error) end end @@ -204,34 +144,29 @@ class VmController < ApplicationController end def vm_action - vm_action = params[:vm_action] - data = params[:vm_action_data] begin - if @vm.queue_action(get_login_user, vm_action, data) - render :json => { :object => "vm", :success => true, :alert => "#{vm_action} was successfully queued." } - else - render :json => { :object => "vm", :success => false, :alert => "#{vm_action} is an invalid action." } - end - rescue - render :json => { :object => "vm", :success => false, :alert => "Error in queueing #{vm_action}." } + alert = svc_vm_action(params[:id], params[:vm_action], + params[:vm_action_data]) + render :json => { :object => "vm", :success => true, :alert => alert } + rescue ActionError => error + json_error("vm", @vm, error) + rescue Exception => error + json_error("vm", @vm, error) end end def cancel_queued_tasks begin - Task.transaction do - @vm.tasks.queued.each { |task| task.cancel} - end - render :json => { :object => "vm", :success => true, :alert => "queued tasks were canceled." } - rescue - render :json => { :object => "vm", :success => true, :alert => "queued tasks cancel failed." } + alert = svc_cancel_queued_tasks(params[:id]) + render :json => { :object => "vm", :success => true, :alert => alert } + rescue Exception => error + json_error("vm", @vm, error) end end def migrate @vm = Vm.find(params[:id]) - @perm_obj = @vm.get_hardware_pool - @redir_obj = @vm + set_perms(@vm.get_hardware_pool) @current_pool_id=@vm.vm_resource_pool.id authorize_admin render :layout => 'popup' @@ -268,53 +203,10 @@ class VmController < ApplicationController end end - # FIXME: move this to an edit_vm task in taskomatic - def _setup_vm_provision(params) - # spaces are invalid in the cobbler name - name = params[:vm][:uuid] - mac = params[:vm][:vnic_mac_addr] - provision = params[:vm][:provisioning_and_boot_settings] - # determine what type of provisioning was selected for the VM - provisioning_type = :pxe_or_hd_type - provisioning_type = :image_type if provision.index "#{Vm::IMAGE_PREFIX}@#{Vm::COBBLER_PREFIX}" - provisioning_type = :system_type if provision.index "#{Vm::PROFILE_PREFIX}@#{Vm::COBBLER_PREFIX}" - - unless provisioning_type == :pxe_or_hd_type - cobbler_name = provision.slice(/(.*):(.*)/, 2) - system = Cobbler::System.find_one(name) - unless system - nic = Cobbler::NetworkInterface.new({'mac_address' => mac}) - - case provisioning_type - when :image_type: - system = Cobbler::System.new("name" => name, "image" => cobbler_name) - when :system_type: - system = Cobbler::System.new("name" => name, "profile" => cobbler_name) - end - - system.interfaces = [nic] - system.save - end - end - end - def pre_new - # if no vm_resource_pool is passed in, find (or auto-create) it based on hardware_pool_id unless params[:vm_resource_pool_id] - unless params[:hardware_pool_id] - flash[:notice] = "VM Resource Pool or Hardware Pool is required." - redirect_to :controller => 'dashboard' - end - @hardware_pool = HardwarePool.find(params[:hardware_pool_id]) - @user = get_login_user - vm_resource_pool = @hardware_pool.sub_vm_resource_pools.select {|pool| pool.name == @user}.first - if vm_resource_pool - params[:vm_resource_pool_id] = vm_resource_pool.id - else - @vm_resource_pool = VmResourcePool.new({:name => vm_resource_pool}) - @vm_resource_pool.tmp_parent = @hardware_pool - @vm_resource_pool_name = @user - end + flash[:notice] = "VM Resource Pool is required." + redirect_to :controller => 'dashboard' end # random MAC @@ -331,50 +223,27 @@ class VmController < ApplicationController unless params[:vm_resource_pool_id] @vm.vm_resource_pool = @vm_resource_pool end - @perm_obj = @vm.vm_resource_pool - @current_pool_id=@perm_obj.id + set_perms(@vm.vm_resource_pool) + @current_pool_id=@vm.vm_resource_pool.id @networks = Network.find(:all).collect{ |net| [net.name, net.id] } _setup_provisioning_options end - def pre_create - params[:vm][:state] = Vm::STATE_PENDING - vm_resource_pool_name = params[:vm_resource_pool_name] - hardware_pool_id = params[:hardware_pool_id] - if vm_resource_pool_name and hardware_pool_id - hardware_pool = HardwarePool.find(hardware_pool_id) - vm_resource_pool = VmResourcePool.new({:name => vm_resource_pool_name}) - vm_resource_pool.create_with_parent(hardware_pool) - params[:vm][:vm_resource_pool_id] = vm_resource_pool.id - end - params[:vm][:forward_vnc] = params[:forward_vnc] - @vm = Vm.new(params[:vm]) - @perm_obj = @vm.vm_resource_pool - @current_pool_id=@perm_obj.id - end - def pre_show - @vm = Vm.find(params[:id]) - @perm_obj = @vm.vm_resource_pool - @current_pool_id=@perm_obj.id - end def pre_edit @vm = Vm.find(params[:id]) - @perm_obj = @vm.vm_resource_pool - @current_pool_id=@perm_obj.id + set_perms(@vm.vm_resource_pool) + @current_pool_id=@vm.vm_resource_pool.id @networks = Network.find(:all).collect{ |net| [net.name, net.id] } _setup_provisioning_options end - def pre_vm_action + def pre_console pre_edit authorize_user end - - private - - def destroy_cobbler_system(vm) - # Destroy the Cobbler system first if it's defined - if vm.uses_cobbler? - system = Cobbler::System.find_one(vm.cobbler_system_name) - system.remove if system - end + # FIXME: remove these when service transition is complete. these are here + # to keep from running permissions checks and other setup steps twice + def tmp_pre_update + end + def tmp_authorize_admin end + end diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb index c62595a..4c7925c 100644 --- a/src/app/models/vm.rb +++ b/src/app/models/vm.rb @@ -220,6 +220,7 @@ class Vm < ActiveRecord::Base self[:boot_device]= BOOT_DEV_HD self[:provisioning]= nil else + # FIXME: Should this case trigger an error instead? self[:boot_device]= BOOT_DEV_NETWORK self[:provisioning]= settings end diff --git a/src/app/services/application_service.rb b/src/app/services/application_service.rb new file mode 100644 index 0000000..e2d0d38 --- /dev/null +++ b/src/app/services/application_service.rb @@ -0,0 +1,57 @@ +# +# 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. +# +# Common infrastructure for business logic for WUI and QMF +# +# We call objects in the mid-level API 'Service' for lack of a better name. +# The Service layer is all in modules that are included by the classes that +# use them in the WUI and the QMF controllers. They set instance variables, +# which automatically become instance variables on the controllers that use +# the Service modules + +module ApplicationService + class PermissionError < RuntimeError; end + class ActionError < RuntimeError; end + + # Including class must provide a GET_LOGIN_USER + + def set_perms(perm_obj) + @perm_obj = perm_obj + @user = get_login_user + @can_view = @perm_obj.can_view(@user) + @can_control_vms = @perm_obj.can_control_vms(@user) + @can_modify = @perm_obj.can_modify(@user) + @can_view_perms = @perm_obj.can_view_perms(@user) + @can_set_perms = @perm_obj.can_set_perms(@user) + end + + def authorized?(privilege, perm_obj=nil) + set_perms(perm_obj) if perm_obj + return false unless @perm_obj + return @perm_obj.has_privilege(@user, privilege) + end + def authorized!(privilege, perm_obj=nil) + unless authorized?(privilege, perm_obj) + raise PermissionError.new( + 'You have insufficient privileges to perform action.') + end + end + +end diff --git a/src/app/services/vm_service.rb b/src/app/services/vm_service.rb new file mode 100644 index 0000000..c44afb7 --- /dev/null +++ b/src/app/services/vm_service.rb @@ -0,0 +1,183 @@ +# +# 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 individual VM's +module VmService + + include ApplicationService + + def svc_show(vm_id) + # from before_filter + @vm = Vm.find(vm_id) + @current_pool_id=@vm.vm_resource_pool.id + authorized!(Privilege::VIEW, at vm.vm_resource_pool) + end + + def svc_create(vm_hash, start_now) + # from before_filter + vm_hash[:state] = Vm::STATE_PENDING + @vm = Vm.new(params[:vm]) + @current_pool_id=@vm.vm_resource_pool.id + authorized!(Privilege::MODIFY, at vm.vm_resource_pool) + + alert = "VM was successfully created." + Vm.transaction do + @vm.save! + vm_provision + @task = VmTask.new({ :user => @user, + :task_target => @vm, + :action => VmTask::ACTION_CREATE_VM, + :state => Task::STATE_QUEUED}) + @task.save! + if start_now + if @vm.get_action_list.include?(VmTask::ACTION_START_VM) + @task = VmTask.new({ :user => @user, + :task_target => @vm, + :action => VmTask::ACTION_START_VM, + :state => Task::STATE_QUEUED}) + @task.save! + alert += " VM Start action queued." + else + alert += " Resources are not available to start VM now." + end + end + end + return alert + end + + def svc_update(vm_id, vm_hash, start_now, restart_now) + # from before_filter + @vm = Vm.find(vm_id) + @current_pool_id=@vm.vm_resource_pool.id + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) + + #needs restart if certain fields are changed + # (since those will only take effect the next startup) + needs_restart = false + unless @vm.get_pending_state == Vm::STATE_STOPPED + Vm::NEEDS_RESTART_FIELDS.each do |field| + unless @vm[field].to_s == vm_hash[field] + needs_restart = true + break + end + end + current_storage_ids = @vm.storage_volume_ids.sort + new_storage_ids = vm_hash[:storage_volume_ids] + new_storage_ids = [] unless new_storage_ids + new_storage_ids = new_storage_ids.sort.collect {|x| x.to_i } + needs_restart = true unless current_storage_ids == new_storage_ids + end + vm_hash[:needs_restart] = 1 if needs_restart + + + alert = "VM was successfully updated." + Vm.transaction do + @vm.update_attributes!(vm_hash) + vm_provision + if start_now + if @vm.get_action_list.include?(VmTask::ACTION_START_VM) + @task = VmTask.new({ :user => @user, + :task_target => @vm, + :action => VmTask::ACTION_START_VM, + :state => Task::STATE_QUEUED}) + @task.save! + alert += " VM Start action queued." + else + alert += " Resources are not available to start VM now." + end + elsif restart_now + if @vm.get_action_list.include?(VmTask::ACTION_SHUTDOWN_VM) + @task = VmTask.new({ :user => @user, + :task_target => @vm, + :action => VmTask::ACTION_SHUTDOWN_VM, + :state => Task::STATE_QUEUED}) + @task.save! + @task = VmTask.new({ :user => @user, + :task_target => @vm, + :action => VmTask::ACTION_START_VM, + :state => Task::STATE_QUEUED}) + @task.save! + alert += " VM Restart action queued." + else + alert += " Restart action was not available." + end + end + end + return alert + end + + def svc_destroy(vm_id) + # from before_filter + @vm = Vm.find(vm_id) + @current_pool_id=@vm.vm_resource_pool.id + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) + + unless @vm.is_destroyable? + raise ActionError.new("Virtual Machine must be stopped to delete it") + end + destroy_cobbler_system(@vm) + @vm.destroy + return "Virtual Machine wa ssuccessfully deleted." + end + + def svc_vm_action(vm_id, vm_action, action_args) + @vm = Vm.find(vm_id) + @current_pool_id=@vm.vm_resource_pool.id + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) + unless @vm.queue_action(@user, vm_action, action_args) + raise ActionError.new("#{vm_action} is an invalid action.") + end + return "#{vm_action} was successfully queued." + end + + def svc_cancel_queued_tasks(vm_id) + @vm = Vm.find(vm_id) + @current_pool_id=@vm.vm_resource_pool.id + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) + + Task.transaction do + @vm.tasks.queued.each { |task| task.cancel} + end + return "Queued tasks were successfully canceled." + end + + def vm_provision + if @vm.uses_cobbler? + # spaces are invalid in the cobbler name + name = @vm.uuid + system = Cobbler::System.find_one(name) + unless system + system = Cobbler::System.new("name" => name, + @vm.cobbler_type => @vm.cobbler_name) + system.interfaces = [Cobbler::NetworkInterface.new( + {'mac_address' => @vm.vnic_mac_addr})] + system.save + end + end + end + + def destroy_cobbler_system(vm) + # Destroy the Cobbler system first if it's defined + if vm.uses_cobbler? + system = Cobbler::System.find_one(vm.cobbler_system_name) + system.remove if system + end + end + +end 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;"} %> -- 1.6.0.6
Ian Main
2009-Apr-21 20:14 UTC
[Ovirt-devel] [PATCH server] Revised attempt at refactoring the vm_controller using the service layer.
So with this service layer.. with it being included and setting local properties of the object, will that be thread safe? Or will we have to do locking around each call? I'm not sure yet if the agent will be threaded.. but it certainly could have a thread per method invocation.. Ian> +module VmService > + > + include ApplicationService > + > + def svc_show(vm_id) > + # from before_filter > + @vm = Vm.find(vm_id) > + @current_pool_id=@vm.vm_resource_pool.id > + authorized!(Privilege::VIEW, at vm.vm_resource_pool) > + end > + > + def svc_create(vm_hash, start_now) > + # from before_filter > + vm_hash[:state] = Vm::STATE_PENDING > + @vm = Vm.new(params[:vm]) > + @current_pool_id=@vm.vm_resource_pool.id > + authorized!(Privilege::MODIFY, at vm.vm_resource_pool) > + > + alert = "VM was successfully created." > + Vm.transaction do > + @vm.save! > + vm_provision > + @task = VmTask.new({ :user => @user, > + :task_target => @vm, > + :action => VmTask::ACTION_CREATE_VM, > + :state => Task::STATE_QUEUED}) > + @task.save! > + if start_now > + if @vm.get_action_list.include?(VmTask::ACTION_START_VM) > + @task = VmTask.new({ :user => @user, > + :task_target => @vm, > + :action => VmTask::ACTION_START_VM, > + :state => Task::STATE_QUEUED}) > + @task.save! > + alert += " VM Start action queued." > + else > + alert += " Resources are not available to start VM now." > + end > + end > + end > + return alert > + end > + > + def svc_update(vm_id, vm_hash, start_now, restart_now) > + # from before_filter > + @vm = Vm.find(vm_id) > + @current_pool_id=@vm.vm_resource_pool.id > + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) > + > + #needs restart if certain fields are changed > + # (since those will only take effect the next startup) > + needs_restart = false > + unless @vm.get_pending_state == Vm::STATE_STOPPED > + Vm::NEEDS_RESTART_FIELDS.each do |field| > + unless @vm[field].to_s == vm_hash[field] > + needs_restart = true > + break > + end > + end > + current_storage_ids = @vm.storage_volume_ids.sort > + new_storage_ids = vm_hash[:storage_volume_ids] > + new_storage_ids = [] unless new_storage_ids > + new_storage_ids = new_storage_ids.sort.collect {|x| x.to_i } > + needs_restart = true unless current_storage_ids == new_storage_ids > + end > + vm_hash[:needs_restart] = 1 if needs_restart > + > + > + alert = "VM was successfully updated." > + Vm.transaction do > + @vm.update_attributes!(vm_hash) > + vm_provision > + if start_now > + if @vm.get_action_list.include?(VmTask::ACTION_START_VM) > + @task = VmTask.new({ :user => @user, > + :task_target => @vm, > + :action => VmTask::ACTION_START_VM, > + :state => Task::STATE_QUEUED}) > + @task.save! > + alert += " VM Start action queued." > + else > + alert += " Resources are not available to start VM now." > + end > + elsif restart_now > + if @vm.get_action_list.include?(VmTask::ACTION_SHUTDOWN_VM) > + @task = VmTask.new({ :user => @user, > + :task_target => @vm, > + :action => VmTask::ACTION_SHUTDOWN_VM, > + :state => Task::STATE_QUEUED}) > + @task.save! > + @task = VmTask.new({ :user => @user, > + :task_target => @vm, > + :action => VmTask::ACTION_START_VM, > + :state => Task::STATE_QUEUED}) > + @task.save! > + alert += " VM Restart action queued." > + else > + alert += " Restart action was not available." > + end > + end > + end > + return alert > + end > + > + def svc_destroy(vm_id) > + # from before_filter > + @vm = Vm.find(vm_id) > + @current_pool_id=@vm.vm_resource_pool.id > + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) > + > + unless @vm.is_destroyable? > + raise ActionError.new("Virtual Machine must be stopped to delete it") > + end > + destroy_cobbler_system(@vm) > + @vm.destroy > + return "Virtual Machine wa ssuccessfully deleted." > + end > + > + def svc_vm_action(vm_id, vm_action, action_args) > + @vm = Vm.find(vm_id) > + @current_pool_id=@vm.vm_resource_pool.id > + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) > + unless @vm.queue_action(@user, vm_action, action_args) > + raise ActionError.new("#{vm_action} is an invalid action.") > + end > + return "#{vm_action} was successfully queued." > + end > + > + def svc_cancel_queued_tasks(vm_id) > + @vm = Vm.find(vm_id) > + @current_pool_id=@vm.vm_resource_pool.id > + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) > + > + Task.transaction do > + @vm.tasks.queued.each { |task| task.cancel} > + end > + return "Queued tasks were successfully canceled." > + end > + > + def vm_provision > + if @vm.uses_cobbler? > + # spaces are invalid in the cobbler name > + name = @vm.uuid > + system = Cobbler::System.find_one(name) > + unless system > + system = Cobbler::System.new("name" => name, > + @vm.cobbler_type => @vm.cobbler_name) > + system.interfaces = [Cobbler::NetworkInterface.new( > + {'mac_address' => @vm.vnic_mac_addr})] > + system.save > + end > + end > + end > + > + def destroy_cobbler_system(vm) > + # Destroy the Cobbler system first if it's defined > + if vm.uses_cobbler? > + system = Cobbler::System.find_one(vm.cobbler_system_name) > + system.remove if system > + end > + end > + > +end > 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;"} %> > -- > 1.6.0.6 > > _______________________________________________ > Ovirt-devel mailing list > Ovirt-devel at redhat.com > https://www.redhat.com/mailman/listinfo/ovirt-devel
David Lutterkort
2009-Apr-23 00:12 UTC
[Ovirt-devel] [PATCH server] Revised attempt at refactoring the vm_controller using the service layer.
On Mon, 2009-04-20 at 20:26 +0000, Scott Seago wrote:> This code is based on David Lutterkort's proposal outlined in > https://www.redhat.com/archives/ovirt-devel/2009-April/msg00086.html > > I've included service-layer methods for show, create, update, destroy, > vm_action, and cancel_queued_tasks. These are the concrete actions > that make sense as part of the API. Most of the remaining > vm_controller methods are pure-WUI actions -- for things such as > generating the web form for a new VM (the subsequent 'create' action > _is_ part of the API).I really like that, and I also like that most controller methods are now very short, and give you a much better impression what happens to set up the view. 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) I've given this some light testing, and as far as I can tell everything works well; so ACK from me. The comments below are minor nits and some ideas on further refactoring that should happen separately.> diff --git a/src/app/controllers/application.rb b/src/app/controllers/application.rb > index fa88773..e5f4d4b 100644 > --- a/src/app/controllers/application.rb > +++ b/src/app/controllers/application.rb> + def authorize_view(msg=nil) > + authorize_action(Privilege::VIEW,msg) > + endThis is unrelated, but it seems odd that we both have authorize_view here and Pool.can_view - I would argue that the Pool.can_XXX methods should go away in favor of authorize_XXX and can_XXX methods in the AppService module (see below) I like though how authorize_view simplifies the places where we check for view permissions.> diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb > index f5f3c9d..93c81ee 100644 > --- a/src/app/controllers/smart_pools_controller.rb > +++ b/src/app/controllers/smart_pools_controller.rb > @@ -199,19 +199,19 @@ class SmartPoolsController < PoolController > def pre_new > @pool = SmartPool.new > @parent = DirectoryPool.get_or_create_user_root(get_login_user) > - @perm_obj = @parent > + set_perms(@parent) > @current_pool_id=@parent.id > end > def pre_create > @pool = SmartPool.new(params[:smart_pool]) > @parent = DirectoryPool.get_or_create_user_root(get_login_user) > - @perm_obj = @parent > + set_perms(@parent) > @current_pool_id=@parent.id > end > def pre_edit > @pool = SmartPool.find(params[:id]) > @parent = @pool.parent > - @perm_obj = @pool > + set_perms(@pool) > @current_pool_id=@pool.idThis code is all over the place, i.e. @current_pool_id = some_pool.id set_perms(some_pool) Why not pull setting @current_pool_id into set_perms ?> diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb > index c62595a..4c7925c 100644 > --- a/src/app/models/vm.rb > +++ b/src/app/models/vm.rb > @@ -220,6 +220,7 @@ class Vm < ActiveRecord::Base > self[:boot_device]= BOOT_DEV_HD > self[:provisioning]= nil > else > + # FIXME: Should this case trigger an error instead? > self[:boot_device]= BOOT_DEV_NETWORK > self[:provisioning]= settings > endShould probably be a separate patch> diff --git a/src/app/services/application_service.rb b/src/app/services/application_service.rb > new file mode 100644 > index 0000000..e2d0d38 > --- /dev/null > +++ b/src/app/services/application_service.rb> +module ApplicationService > + class PermissionError < RuntimeError; end > + class ActionError < RuntimeError; end > + > + # Including class must provide a GET_LOGIN_USER > + > + def set_perms(perm_obj) > + @perm_obj = perm_obj > + @user = get_login_user > + @can_view = @perm_obj.can_view(@user) > + @can_control_vms = @perm_obj.can_control_vms(@user) > + @can_modify = @perm_obj.can_modify(@user) > + @can_view_perms = @perm_obj.can_view_perms(@user) > + @can_set_perms = @perm_obj.can_set_perms(@user) > + endAs 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> 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
David Lutterkort
2009-Apr-24 01:07 UTC
[Ovirt-devel] [PATCH server] Revised attempt at refactoring the vm_controller using the service layer.
On Mon, 2009-04-20 at 20:26 +0000, Scott Seago wrote:> This code is based on David Lutterkort's proposal outlined in > https://www.redhat.com/archives/ovirt-devel/2009-April/msg00086.htmlACK. Works for me. David