Hi Jan, Looking at the new method here, and it seems there will be a slight issue with displaying the correct message for starting an instance.
`condormatic_instance_create(@task)` will checks quota and saves the instance should there be sufficient quota left. This then initiates the instance_observer, which increments the number of instances, for each associated quota. In the new method, `check_can_start_instance(@task)` is called after this method. Which essentially checks to see if another instance can be started. Meaning that when a quota is at it's maximum limit after condormatic_instance_create(@task) has updated. The instance will be started but flash[:warning] = "Quota Exceeded: Instance will not start until you have free quota" will be shown. Ideally condormatic_instance_create(@task) should throw an exception, when it can not start instance due to insufficient quota, this way we could inspect it to find out what the issue was. For now I suggest we do the Quota check before calling condormatic_instance_create(@task). Thanks Martyn ----- Original Message ----- From: [email protected] To: [email protected] Sent: Tuesday, December 21, 2010 9:41:15 AM Subject: [deltacloud-devel] [PATCH aggregator 4/8] Cleaned up instance create method From: Jan Provaznik <[email protected]> Moved to transaction and dropped some nested 'if's --- .../controllers/resources/instances_controller.rb | 73 +++++++++---------- src/app/views/resources/instances/new.haml | 2 + 2 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/app/controllers/resources/instances_controller.rb b/src/app/controllers/resources/instances_controller.rb index bf95731..1976561 100644 --- a/src/app/controllers/resources/instances_controller.rb +++ b/src/app/controllers/resources/instances_controller.rb @@ -10,15 +10,7 @@ class Resources::InstancesController < ApplicationController return end - @pools = Pool.list_for_user(@current_user, Privilege::INSTANCE_MODIFY) - @realms = Realm.find(:all, :conditions => { :provider_id => nil }) - @hardware_profiles = HardwareProfile.all( - :include => :architecture, - :conditions => { - :provider_id => nil, - 'hardware_profile_properties.value' => @instance.template.architecture - } - ) + init_new_instance_attrs end def select_template @@ -41,36 +33,27 @@ class Resources::InstancesController < ApplicationController @instance.state = Instance::STATE_NEW @instance.owner = current_user - require_privilege(Privilege::INSTANCE_MODIFY, - Pool.find(@instance.pool_id)) - #FIXME: This should probably be in a transaction - if @instance.save - @task = InstanceTask.new({:user => current_user, - :task_target => @instance, - :action => InstanceTask::ACTION_CREATE}) - if @task.save - condormatic_instance_create(@task) - if Quota.can_start_instance?(@instance, nil) - flash[:notice] = "Instance added." - else - flash[:warning] = "Quota Exceeded: Instance will not start until you have free quota" - end - else - @pool = @instance.pool - render :new - end - redirect_to resources_instances_path - else - @pools = Pool.list_for_user(@current_user, Privilege::INSTANCE_MODIFY) - @realms = Realm.find(:all, :conditions => { :provider_id => nil }) - @hardware_profiles = HardwareProfile.all( - :include => :architecture, - :conditions => { - :provider_id => nil, - 'hardware_profile_properties.value' => @instance.template.architecture - } - ) + begin + require_privilege(Privilege::INSTANCE_MODIFY, + Pool.find(@instance.pool_id)) + @instance.transaction do + @instance.save! + @task = InstanceTask.create!({:user => current_user, + :task_target => @instance, + :action => InstanceTask::ACTION_CREATE}) + condormatic_instance_create(@task) + end + rescue + init_new_instance_attrs + flash[:warning] = "Failed to launch instance: #{$!}" render :new + else + if Quota.can_start_instance?(@instance, nil) + flash[:notice] = "Instance added." + else + flash[:warning] = "Quota Exceeded: Instance will not start until you have free quota" + end + redirect_to resources_instances_path end end @@ -100,4 +83,18 @@ class Resources::InstancesController < ApplicationController :order => (params[:order_field] || 'name') +' '+ (params[:order_dir] || 'asc') ) end + + private + + def init_new_instance_attrs + @pools = Pool.list_for_user(@current_user, Privilege::INSTANCE_MODIFY) + @realms = Realm.find(:all, :conditions => { :provider_id => nil }) + @hardware_profiles = HardwareProfile.all( + :include => :architecture, + :conditions => { + :provider_id => nil, + 'hardware_profile_properties.value' => @instance.template.architecture + } + ) + end end diff --git a/src/app/views/resources/instances/new.haml b/src/app/views/resources/instances/new.haml index 9d1d331..13c973c 100644 --- a/src/app/views/resources/instances/new.haml +++ b/src/app/views/resources/instances/new.haml @@ -1,3 +1,4 @@ += error_messages_for 'instance' %h2 Launch instance - form_for @instance, :url => resources_instances_path do = hidden_field :instance, :template_id @@ -12,6 +13,7 @@ = label :instance, :pool - if @instance.pool = text_field_tag :pool_name, @instance.pool.name, :disabled => true + = hidden_field :instance, :pool_id - else = select :instance, :pool_id, @pools.map {|p| [ p.name, p.id ]}, { :include_blank => true } %li -- 1.7.2.3 _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
