[Ovirt-devel] Re: [PATCH] transactions in models and controllers
Hugh O. Brock
hbrock at redhat.com
Tue May 27 21:31:07 UTC 2008
On Tue, May 27, 2008 at 01:16:21PM -0500, steve linabery wrote:
> On Sun, May 25, 2008 at 3:30 PM, steve linabery <slinabery at gmail.com> wrote:
> > Hi Ovirt,
> >
> > I believe this will make everything currently in app/models and
> > app/controllers transactional where necessary.
> >
> > As an aside, please note that these (and other recent) transaction
> > additions create the need for some exception handling that is not
> > currently present.
> >
> > Good day,
> > Steve
> >
>
> Hi Ovirt,
>
> Scott and I decided it would be better to wrap the insert_refresh_task
> with the create/update of StoragePool. So this version includes those
> changes as well.
>
> Thanks!
> Steve
> From d1abfa2e37701f97768ed5b0b030467b5fb06c82 Mon Sep 17 00:00:00 2001
> From: Steve Linabery <slinabery at gmail.com>
> Date: Tue, 27 May 2008 13:12:06 -0500
> Subject: [PATCH] Adds remaining transactions to models and controllers.
>
>
> Signed-off-by: Steve Linabery <slinabery at gmail.com>
> ---
> wui/src/app/controllers/permission_controller.rb | 2 +-
> wui/src/app/controllers/storage_controller.rb | 24 ++++++++++++---------
> wui/src/app/controllers/vm_controller.rb | 24 ++++++++++++++-------
> wui/src/app/models/hardware_pool.rb | 16 +++++++++-----
> wui/src/app/models/task.rb | 2 +-
> 5 files changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/wui/src/app/controllers/permission_controller.rb b/wui/src/app/controllers/permission_controller.rb
> index 1787a27..3cbaaf6 100644
> --- a/wui/src/app/controllers/permission_controller.rb
> +++ b/wui/src/app/controllers/permission_controller.rb
> @@ -74,7 +74,7 @@ class PermissionController < ApplicationController
> role = params[:user_role]
> permission_ids_str = params[:permission_ids]
> permission_ids = permission_ids_str.split(",").collect {|x| x.to_i}
> -
> +
> Permission.transaction do
> permissions = Permission.find(:all, :conditions => "id in (#{permission_ids.join(', ')})")
> permissions.each do |permission|
> diff --git a/wui/src/app/controllers/storage_controller.rb b/wui/src/app/controllers/storage_controller.rb
> index 618acb4..d154a80 100644
> --- a/wui/src/app/controllers/storage_controller.rb
> +++ b/wui/src/app/controllers/storage_controller.rb
> @@ -112,8 +112,10 @@ class StorageController < ApplicationController
>
> def create
> begin
> - @storage_pool.save!
> - insert_refresh_task
> + StoragePool.transaction do
> + @storage_pool.save!
> + insert_refresh_task
> + end
> render :json => { :object => "storage_pool", :success => true,
> :alert => "Storage Pool was successfully created." }
> rescue
> @@ -128,14 +130,16 @@ class StorageController < ApplicationController
>
> def update
> # FIXME: handle save! properly, in conjunction w/ update_attributes, json, etc
> - if @storage_pool.update_attributes(params[:storage_pool])
> - if insert_refresh_task
> - storage_url = url_for(:controller => "storage", :action => "show", :id => @storage_pool)
> - flash[:notice] = '<a class="show" href="%s">%s</a> was successfully updated.' % [ storage_url , at storage_pool.ip_addr]
> - redirect_to :action => 'show', :id => @storage_pool
> - else
> - render :action => 'edit'
> - end
> + StoragePool.transaction do
> + @storage_pool.update_attributes!(params[:storage_pool])
> + insert_refresh_task
> + transaction_succeeded = true
> + end
> +
> + if transaction_succeeded
> + storage_url = url_for(:controller => "storage", :action => "show", :id => @storage_pool)
> + flash[:notice] = '<a class="show" href="%s">%s</a> was successfully updated.' % [ storage_url , at storage_pool.ip_addr]
> + redirect_to :action => 'show', :id => @storage_pool
> else
> render :action => 'edit'
> end
> diff --git a/wui/src/app/controllers/vm_controller.rb b/wui/src/app/controllers/vm_controller.rb
> index ac29beb..856c494 100644
> --- a/wui/src/app/controllers/vm_controller.rb
> +++ b/wui/src/app/controllers/vm_controller.rb
> @@ -40,12 +40,14 @@ class VmController < ApplicationController
>
> def create
> begin
> - @vm.save!
> - @task = VmTask.new({ :user => @user,
> - :vm_id => @vm.id,
> - :action => VmTask::ACTION_CREATE_VM,
> - :state => Task::STATE_QUEUED})
> - @task.save!
> + Vm.transaction do
> + @vm.save!
> + @task = VmTask.new({ :user => @user,
> + :vm_id => @vm.id,
> + :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)
> @@ -150,8 +152,14 @@ class VmController < ApplicationController
> end
>
> def cancel_queued_tasks
> - @vm.get_queued_tasks.each { |task| task.cancel}
> - flash[:notice] = "queued tasks canceled."
> + begin
> + Task.transaction do
> + @vm.get_queued_tasks.each { |task| task.cancel}
> + end
> + flash[:notice] = "queued tasks canceled."
> + rescue
> + flash[:notice] = "cancel queued tasks failed."
> + end
> redirect_to :controller => 'vm', :action => 'show', :id => params[:id]
> end
>
> diff --git a/wui/src/app/models/hardware_pool.rb b/wui/src/app/models/hardware_pool.rb
> index df22b90..b1adc8f 100644
> --- a/wui/src/app/models/hardware_pool.rb
> +++ b/wui/src/app/models/hardware_pool.rb
> @@ -60,17 +60,21 @@ class HardwarePool < Pool
>
> def move_hosts(host_ids, target_pool_id)
> hosts = Host.find(:all, :conditions => "id in (#{host_ids.join(', ')})")
> - hosts.each do |host|
> - host.hardware_pool_id = target_pool_id
> - host.save!
> + transaction do
> + hosts.each do |host|
> + host.hardware_pool_id = target_pool_id
> + host.save!
> + end
> end
> end
>
> def move_storage(storage_pool_ids, target_pool_id)
> storage_pools = StoragePool.find(:all, :conditions => "id in (#{storage_pool_ids.join(', ')})")
> - storage_pools.each do |storage_pool|
> - storage_pool.hardware_pool_id = target_pool_id
> - storage_pool.save!
> + transaction do
> + storage_pools.each do |storage_pool|
> + storage_pool.hardware_pool_id = target_pool_id
> + storage_pool.save!
> + end
> end
> end
>
> diff --git a/wui/src/app/models/task.rb b/wui/src/app/models/task.rb
> index a3e82c9..7960056 100644
> --- a/wui/src/app/models/task.rb
> +++ b/wui/src/app/models/task.rb
> @@ -31,7 +31,7 @@ class Task < ActiveRecord::Base
>
> def cancel
> self[:state] = STATE_CANCELED
> - save
> + save!
> end
>
> def self.working_tasks(user = nil)
> --
ACK. Tested, appears to function normally, committed. Obviously we need to add error handling to deal with some failures here, but this is a good start.
Thanks!
--Hugh
More information about the ovirt-devel
mailing list