Ovirt devel - Apr 2009 - HostController -> Service

These patches factor the business logic out of the HostController into a
HostService. The actual refactoring is in 3/3.

The first patch is a minor fix for the Rakefile, so that rake doc:app
works, since I wanted to look at my nice rdoc for HostService. Since the
service modules do a lot with instance variabes, we should document them a
little more formally than the rst of the API.

The second patch makes dealing with auth errors in the WUI a little more
convenient; if everybody agrees, I'll change the rest of the code to the
new style, which will make it possible to pass a hash exception -> error
message in.

David

---
 src/Rakefile |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/src/Rakefile b/src/Rakefile
index 74b9d18..eebf798 100644
--- a/src/Rakefile
+++ b/src/Rakefile
@@ -1,3 +1,4 @@
+# -*- ruby -*-
 # Add your own tasks in files placed in lib/tasks ending in .rake,
 # for example lib/tasks/capistrano.rake, and they will automatically be
available to Rake.
 
@@ -8,14 +9,18 @@ require 'rake/testtask'
 require 'rake/rdoctask'
 
 require 'tasks/rails'
-require 'gettext/utils'
+# Do not require 'gettext/utils' here, it breaks rdoc,
+# require it in tasks that actually need it instead
+
 desc "Create mo-files for L10n" 
 task :makemo do
+ require 'gettext/utils'
  GetText.create_mofiles(true, "po", "locale")
 end
 
 desc "Update pot/po files to match new version." 
 task :updatepo do
+ require 'gettext/utils'
  MY_APP_TEXT_DOMAIN = "ovirt" 
  MY_APP_VERSION     = "ovirt 0.0.1" 
  GetText.update_pofiles(MY_APP_TEXT_DOMAIN,
-- 
1.6.0.6

This makes dealing with auth errors a little more concise. Eventually, I
would like to change the signature of this method to be something like

  handle_auth_error(exc_msgs={}, &block)

so that we can pass in a mapping exception class => error message, i.e. a
typical call might be

  handle_auth_error(
    ActiveRecord::RecordNotFound => "The object does not exist",
    ActiveRecord::StaleObjectError => "Midair collision, try
again") do
    .. do stuff ..
  end

To be compatible with existing code using it, it doesn't do that right now,
and behaves just as before if called with a string.
---
 src/app/controllers/application.rb |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/src/app/controllers/application.rb
b/src/app/controllers/application.rb
index e5f4d4b..a36d2fd 100644
--- a/src/app/controllers/application.rb
+++ b/src/app/controllers/application.rb
@@ -115,7 +115,17 @@ class ApplicationController < ActionController::Base
       true
     end
   end
-  def handle_auth_error(msg)
+  def handle_auth_error(msg=nil, &block)
+    if block_given? && !msg.nil?
+      raise ArgumentError, "You can only provide a msg or a block, not
both"
+    end
+    if block_given?
+      begin
+        return yield
+      rescue PermissionError => perm_error
+        msg = perm_error.message
+      end
+    end
     respond_to do |format|
       format.html do
         @title = "Access denied"
-- 
1.6.0.6

The business logic is now in HostService.

The main functional difference is that svc_list filters the host list by
permission.

There's a couple things I am not clear on:

(1) addhost should use PoolService somehow, need to look at Scott's
    latest patch

(2) the way we handle unsupported actions is strange: we define them and
    kick out a nice error - in reality, people can only get there either if
    we have a bug somewhere or if they did URL surgery; either way, they
    deserve a gory Rails error report. We should therefore remove the
    actions and associated nice error messages
---
 src/app/controllers/host_controller.rb |  122 +++++++++++++------------------
 src/app/services/host_service.rb       |  119 +++++++++++++++++++++++++++++++
 2 files changed, 170 insertions(+), 71 deletions(-)
 create mode 100644 src/app/services/host_service.rb

diff --git a/src/app/controllers/host_controller.rb
b/src/app/controllers/host_controller.rb
index 994b5e2..999a104 100644
--- a/src/app/controllers/host_controller.rb
+++ b/src/app/controllers/host_controller.rb
@@ -19,8 +19,13 @@
 
 class HostController < ApplicationController
 
-  EQ_ATTRIBUTES = [ :state, :arch, :hostname, :uuid,
-                    :hardware_pool_id ]
+  include HostService
+
+  # GETs should be safe (see http://www.w3.org/2001/tag/doc/whenToUseGet.html)
+  verify :method => [:post, :put], :only => [ :create, :update ],
+         :redirect_to => { :action => :list }
+  verify :method => [:post, :delete], :only => :destroy,
+         :redirect_to => { :action => :list }
 
   def index
     list
@@ -30,30 +35,13 @@ class HostController < ApplicationController
     end
   end
 
-  before_filter :pre_action, :only => [:host_action, :enable, :disable,
:clear_vms, :edit_network]
-  before_filter :pre_addhost, :only => [:addhost]
-
-  # GETs should be safe (see http://www.w3.org/2001/tag/doc/whenToUseGet.html)
-  verify :method => [:post, :put], :only => [ :create, :update ],
-         :redirect_to => { :action => :list }
-  verify :method => [:post, :delete], :only => :destroy,
-         :redirect_to => { :action => :list }
-
-   def list
-     conditions = []
-     EQ_ATTRIBUTES.each do |attr|
-       if params[attr]
-         conditions << "#{attr} = :#{attr}"
-       end
-     end
-     @hosts = Host.find(:all,
-                        :conditions => [conditions.join(" and "),
params],
-                        :order => "id")
-   end
-
+  def list
+    svc_list(params)
+  end
 
   def show
-    if authorize_view
+    handle_auth_error do
+      svc_show(params[:id])
       respond_to do |format|
         format.html { render :layout => 'selection' }
         format.xml { render :xml => @host.to_xml(:include => [ :cpus ] )
}
@@ -62,8 +50,8 @@ class HostController < ApplicationController
   end
 
   def quick_summary
-    pre_show
-    if authorize_view
+    handle_auth_error do
+      svc_show(id)
       render :layout => false
     end
   end
@@ -73,15 +61,15 @@ class HostController < ApplicationController
   end
 
   def addhost
-    @hardware_pool = Pool.find(params[:hardware_pool_id])
-    render :layout => 'popup'
-  end
-
-  def pre_addhost
+    # FIXME: This probably should go into PoolService.svc_modify
+    # Old pre_addhost
     @pool = Pool.find(params[:hardware_pool_id])
     @parent = @pool.parent
     set_perms(@pool)
     authorize_admin
+    # Old addhost
+    @hardware_pool = Pool.find(params[:hardware_pool_id])
+    render :layout => 'popup'
   end
 
   def add_to_smart_pool
@@ -116,53 +104,51 @@ class HostController < ApplicationController
   end
 
   def disable
-    set_disabled(1)
+    handle_auth_error do
+      svc_enable(params[:id], "disabled")
+      render :json => {
+        :object => :host,
+        :alert => "Host was successfully disabled",
+        :success => true
+      }
+    end
   end
+
   def enable
-    set_disabled(0)
-  end
-
-  def set_disabled(value)
-    operation = value == 1 ? "disabled" : "enabled"
-    begin
-      @host.is_disabled = value
-      @host.save!
-      @json_hash[:alert]="Host was successfully #{operation}"
-      @json_hash[:success]=true
-    rescue
-      @json_hash[:alert]="Error setting host to #{operation}"
-      @json_hash[:success]=false
+    handle_auth_error do
+      svc_enable(params[:id], "enabled")
+      render :json => {
+        :object => :host,
+        :alert => "Host was successfully enabled",
+        :success => true
+      }
     end
-    render :json => @json_hash
   end
 
   def clear_vms
-    begin
-      Host.transaction do
-        task = HostTask.new({ :user        => get_login_user,
-                              :task_target => @host,
-                              :action      => HostTask::ACTION_CLEAR_VMS,
-                              :state       => Task::STATE_QUEUED})
-        task.save!
-        @host.is_disabled = true
-        @host.save!
-      end
-      @json_hash[:alert]="Clear VMs action was successfully queued."
-      @json_hash[:success]=true
-    rescue
-      @json_hash[:alert]="Error in queueing Clear VMs action."
-      @json_hash[:success]=false
+    handle_auth_error do
+      svc_clear_vms(params[:id])
+      render :json => {
+        :object => :host,
+        :alert => "Clear VMs action was successfully queued.",
+        :success => true
+      }
     end
-    render :json => @json_hash
   end
 
   def edit_network
-    render :layout => 'popup'
+    handle_auth_error do
+      svc_modify(params[:id])
+      render :layout => 'popup'
+    end
   end
 
   def bondings_json
-    bondings = Host.find(params[:id]).bondings
-    render :json => bondings.collect{ |x| {:id => x.id, :name =>
x.name} }
+    handle_auth_error do
+      svc_show(params[:id])
+      bondings = @host.bondings
+      render :json => bondings.collect{ |x| {:id => x.id, :name =>
x.name} }
+    end
   end
 
   private
@@ -180,12 +166,6 @@ class HostController < ApplicationController
     flash[:notice] = 'Hosts may not be edited via the web UI'
     redirect_to :action=> 'show', :id => @host
   end
-  def pre_action
-    @host = Host.find(params[:id])
-    set_perms(@host.hardware_pool)
-    @json_hash = { :object => :host }
-    authorize_admin
-  end
   def pre_show
     @host = Host.find(params[:id])
     set_perms(@host.hardware_pool)
diff --git a/src/app/services/host_service.rb b/src/app/services/host_service.rb
new file mode 100644
index 0000000..074d830
--- /dev/null
+++ b/src/app/services/host_service.rb
@@ -0,0 +1,119 @@
+#--
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by 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.
+#++
+# Business logic around hosts aka nodes
+module HostService
+
+  include ApplicationService
+
+  # Host attributes on which we filter with '='
+  EQ_ATTRIBUTES = [ :state, :arch, :hostname, :uuid,
+                    :hardware_pool_id ]
+
+  # List hosts matching criteria described by +params+; only the entries
+  # for keys in +EQ_ATTRIBUTES+ are used
+  #
+  # === Instance variables
+  # [<tt>@hosts</tt>] stores list of hosts matching criteria
+  # === Required permissions
+  # [<tt>Privilege::VIEW</tt>] no exception raised,
<tt>@hosts</tt>
+  #                            is filtered by privilege
+  def svc_list(params)
+    conditions = []
+    EQ_ATTRIBUTES.each do |attr|
+      if params[attr]
+        conditions << "hosts.#{attr} = :#{attr}"
+      end
+    end
+    # Add permission check
+    params = params.dup
+    params[:user] = get_login_user
+    params[:priv] = Privilege::VIEW
+    conditions << "privileges.name=:priv"
+    conditions << "permissions.uid=:user"
+    incl = [{ :hardware_pool => { :permissions => { :role =>
:privileges}}}]
+    @hosts = Host.find(:all,
+                       :include => incl,
+                       :conditions => [conditions.join(" and "),
params],
+                       :order => "hosts.id")
+  end
+
+  # Load the Host with +id+ for viewing
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::VIEW</tt>] on host's HardwarePool
+  def svc_show(id)
+    lookup(id, Privilege::VIEW)
+  end
+
+  # Load the Host with +id+ for editing
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
+  def svc_modify(id)
+    lookup(id, Privilege::MODIFY)
+  end
+
+  # Set the disabled state of the Host with +id+ to
<tt>:enabled</tt>
+  # or <tt>:disabled</tt>
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
+  def svc_enable(id, state)
+    ind = ["enabled", "disabled"].index(state.to_s)
+    if ind.nil?
+      raise ArgumentError, "STATE must be 'enabled' or
'disabled'"
+    end
+    svc_modify(id)
+    @host.is_disabled = ind
+    @host.save!
+  end
+
+  # Queue task to migrate all VM's off the Host with +id+, and mark the
+  # host as disabled
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
+  def svc_clear_vms(id)
+    svc_modify(id)
+    Host.transaction do
+      task = HostTask.new({ :user        => get_login_user,
+                            :task_target => @host,
+                            :action      => HostTask::ACTION_CLEAR_VMS,
+                            :state       => Task::STATE_QUEUED})
+      task.save!
+      @host.is_disabled = true
+      @host.save!
+    end
+  end
+
+  private
+  def lookup(id, priv)
+    @host = Host.find(id)
+    authorized!(priv, @host.hardware_pool)
+  end
+end
-- 
1.6.0.6