[PATCH 1/2] Cleanup of deployments create action

jprovazn at redhat.com jprovazn at redhat.com
Fri May 4 09:43:06 UTC 2012


From: Jan Provaznik <jprovazn at redhat.com>

https://www.aeolusproject.org/redmine/issues/3204
---
 src/app/controllers/deployments_controller.rb      |   94 ++++++++--------
 src/app/models/deployment.rb                       |  120 +++++++++++---------
 src/config/locales/en.yml                          |    1 +
 .../controllers/deployments_controller_spec.rb     |    1 +
 src/spec/models/deployment_spec.rb                 |   25 ++--
 5 files changed, 126 insertions(+), 115 deletions(-)

diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb
index 3cdc9c6..e982b85 100644
--- a/src/app/controllers/deployments_controller.rb
+++ b/src/app/controllers/deployments_controller.rb
@@ -116,63 +116,49 @@ class DeploymentsController < ApplicationController
   end
 
   def create
-    launch_parameters_encoded = params.delete(:launch_parameters_encoded)
-    unless launch_parameters_encoded.blank?
-      decoded_parameters = JSON.load(Base64.decode64(launch_parameters_encoded))
-      params[:deployment][:launch_parameters] = decoded_parameters
-    end
     @deployment = Deployment.new(params[:deployment])
     @pool = @deployment.pool
     require_privilege(Privilege::CREATE, Deployment, @pool)
+
+    if params[:launch_parameters_encoded].present?
+      @deployment.launch_parameters = JSON.load(
+        Base64.decode64(params[:launch_parameters_encoded]))
+    end
+
     init_new_deployment_attrs
-    @deployment.deployable_xml = @deployable.xml if @deployable && @deployable.xml
-    if params.has_key?(:deployable_id)
-      require_privilege(Privilege::USE, @deployable)
+    require_privilege(Privilege::USE, @deployable)
+    @deployment.deployable_xml = @deployable.xml
+    @deployment.owner = current_user
+
+    if params.delete(:commit) == 'back'
+      load_assemblies_services
+      view = @deployment.launch_parameters.blank? ?
+        'launch_new' : 'launch_time_params'
+      render view
+      return
     end
-    img, img2, missing, d_errors = @deployable.get_image_details
-    respond_to do |format|
-      unless d_errors.empty?
-        @pool = @deployment.pool
-        flash.now[:warning] = t "deployments.flash.warning.failed_to_launch"
-        flash[:error] = d_errors
-        format.html { render :action => 'overview' }
-        format.js { launch_new }
-        format.json { render :json => d_errors, :status => :unprocessable_entity }
-      else
+    return unless check_deployable_images
 
-        @deployment.owner = current_user
-        load_assemblies_services
-        if params.delete(:commit) == 'back'
-          view = launch_parameters_encoded.blank? ? 'launch_new' : 'launch_time_params'
-          render view and return
+    respond_to do |format|
+      if @deployment.create_and_launch(current_user)
+        format.html do
+          flash[:notice] = t "deployments.flash.notice.launched"
+          redirect_to deployment_path(@deployment)
         end
-
-        if @deployment.save
-          status = @deployment.launch(current_user)
-          if status[:errors].empty?
-            flash[:notice] = t "deployments.flash.notice.launched"
-          else
-            flash[:error] = {
-              :summary  => t("deployments.flash.error.failed_to_launch_assemblies"),
-              :failures => status[:errors],
-              :successes => status[:successes]
-            }
-          end
-          format.html { redirect_to deployment_path(@deployment) }
-          format.js do
-            @deployment_properties = @deployment.properties
-            render :partial => 'properties'
-          end
-          format.json { render :json => @deployment, :status => :created }
-        else
-          # We need @pool to re-display the form
-          @pool = @deployment.pool
+        format.js do
+          @deployment_properties = @deployment.properties
+          render :partial => 'properties'
+        end
+        format.json { render :json => @deployment, :status => :created }
+      else
+        # TODO: put deployment's errors into flash or display inside page?
+        format.html do
           flash.now[:warning] = t "deployments.flash.warning.failed_to_launch"
-          init_new_deployment_attrs
-          format.html { render :action => 'overview' }
-          format.js { launch_new }
-          format.json { render :json => @deployment.errors, :status => :unprocessable_entity }
+          render :action => 'overview'
         end
+        format.js { render :partial => 'overview' }
+        format.json { render :json => @deployment.errors,
+                             :status => :unprocessable_entity }
       end
     end
   end
@@ -433,4 +419,16 @@ class DeploymentsController < ApplicationController
     end
   end
 
+  def check_deployable_images
+    image_details, images, missing_images, deployable_errors = @deployable.get_image_details
+    return true if deployable_errors.empty?
+    respond_to do |format|
+      flash.now[:warning] = t "deployments.flash.warning.failed_to_launch"
+      flash[:error] = deployable_errors
+      format.html { render :action => 'overview' }
+      format.js { render :partial => 'overview' }
+      format.json { render :json => deployable_errors, :status => :unprocessable_entity }
+    end
+    false
+  end
 end
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index 4ad2082..d7956ab 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -206,26 +206,33 @@ class Deployment < ActiveRecord::Base
     @launch_parameters = launch_parameters
   end
 
-  def launch(user)
-    # first, create the instance and instance_parameter objects
-    # then, find a single provider account where all instances can launch
-    # then, if a config server is being used
-    #   then generate the instance configs for each instance,
-    # finally, launch the instances
-    #
-    # TODO: need to be able to handle deployable-level errors
-    #
+  def create_and_launch(user)
+    begin
+      rollback_active_record_state! do
+        # deployment's attrs are not reset on rollback,
+        # rollback_active_record_state! takes care of this
+        transaction do
+          create_instances_with_params!(user)
+          launch!(user)
+        end
+      end
+      return true
+    rescue
+      errors.add(:base, $!.message)
+      logger.error $!.message
+      logger.error $!.backtrace.join("\n    ")
+    end
+    false
+  end
+
+  def launch!(user)
     self.state = STATE_PENDING
     save!
-    status = { :errors => {}, :successes => {} }
-    status[:errors] = create_instances_with_params(user)
-    all_inst_match, account, errors = find_match_with_common_account
+    all_inst_match, account, errs = find_match_with_common_account
 
     unless all_inst_match
-      status[:errors][name] = errors
-      # set CREATE_FAILED for all newly created instances
-      instances.each {|i| i.update_attribute(:state, Instance::STATE_CREATE_FAILED)}
-      return status
+      raise I18n.t('deployments.errors.match_not_found',
+                   :errors => errs.join(", "))
     end
 
     if deployable_xml.requires_config_server?
@@ -233,24 +240,34 @@ class Deployment < ActiveRecord::Base
       # instances (and not each individual instance) in order to do parameter
       # dependency resolution across the set
       config_server = account.config_server
-      instance_configs = ConfigServerUtil.instance_configs(self, instances, config_server)
+      instance_configs = ConfigServerUtil.instance_configs(self,
+                                                           instances,
+                                                           config_server)
     else
       config_server = nil
       instance_configs = {}
     end
 
+    # TODO: in follow up patch there should be only delayed job call
+    # on this place
+    # For now, we just call DC create intance method on foreground, if an error
+    # occurs (for example DC API is not running, or config server is not
+    # running), then CREATE_FAILED is set in instance.launch!
     instances.each do |instance|
       match = all_inst_match.find{|m| m.instance.id == instance.id}
       begin
-        instance.launch!(match, user, config_server, instance_configs[instance.uuid])
-        status[:successes][instance.name] = 'launched'
+        instance.launch!(match,
+                         user,
+                         config_server,
+                         instance_configs[instance.uuid])
       rescue
-        status[:errors][instance.name] = $!.message.to_s.split("\n").first
+        errors.add(:base,
+                   "#{instance.name}: #{$!.message.to_s.split("\n").first}")
         logger.error $!.message
         logger.error $!.backtrace.join("\n    ")
       end
     end
-    status
+    true
   end
 
   def self.list(order_field, order_dir)
@@ -535,47 +552,40 @@ class Deployment < ActiveRecord::Base
     self[:pool_family_id] = pool.pool_family_id
   end
 
-  def create_instances_with_params(user)
+  def create_instances_with_params!(user)
     errors = {}
     deployable_xml.assemblies.each do |assembly|
-      begin
-        hw_profile = permissioned_frontend_hwprofile(user, assembly.hwp)
-        raise I18n.t('deployments.flash.error.no_hwp_permission', :hwp => assembly.hwp) unless hw_profile
-        Instance.transaction do
-          instance = Instance.create!(
-            :deployment => self,
-            :name => "#{name}/#{assembly.name}",
-            :frontend_realm => frontend_realm,
-            :pool => pool,
-            :image_uuid => assembly.image_id,
-            :image_build_uuid => assembly.image_build,
-            :assembly_xml => assembly.to_s,
-            :state => Instance::STATE_NEW,
-            :owner => user,
-            :hardware_profile => hw_profile
-          )
-          assembly.services.each do |service|
-            service.parameters.each do |parameter|
-              if not parameter.reference?
-                param = InstanceParameter.create!(
-                  :instance => instance,
-                  :service => service.name,
-                  :name => parameter.name,
-                  :type => parameter.type,
-                  :value => parameter.value
-                )
-              end
+      hw_profile = permissioned_frontend_hwprofile(user, assembly.hwp)
+      raise I18n.t('deployments.flash.error.no_hwp_permission', :hwp => assembly.hwp) unless hw_profile
+      Instance.transaction do
+        instance = Instance.create!(
+          :deployment => self,
+          :name => "#{name}/#{assembly.name}",
+          :frontend_realm => frontend_realm,
+          :pool => pool,
+          :image_uuid => assembly.image_id,
+          :image_build_uuid => assembly.image_build,
+          :assembly_xml => assembly.to_s,
+          :state => Instance::STATE_NEW,
+          :owner => user,
+          :hardware_profile => hw_profile
+        )
+        assembly.services.each do |service|
+          service.parameters.each do |parameter|
+            if not parameter.reference?
+              param = InstanceParameter.create!(
+                :instance => instance,
+                :service => service.name,
+                :name => parameter.name,
+                :type => parameter.type,
+                :value => parameter.value
+              )
             end
           end
-          self.instances << instance
         end
-      rescue
-        logger.error $!.message
-        logger.error $!.backtrace.join("\n    ")
-        errors[assembly.name] = $!.message.to_s.split("\n").first
+        self.instances << instance
       end
     end
-    errors
   end
 
   def set_new_state
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index beb8505..793f448 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -308,6 +308,7 @@ en:
       cannot_stop: "stop cannot be performed on this instance."
       all_stopped: "All instances must be stopped or running"
       not_valid_deployable_xml: "seems to be not valid deployable XML: %{msg}"
+      match_not_found: "Match not found: %{errors}"
     preset_filters:
       all_deployments: All Deployments
   instances:
diff --git a/src/spec/controllers/deployments_controller_spec.rb b/src/spec/controllers/deployments_controller_spec.rb
index 54b1477..69057ba 100644
--- a/src/spec/controllers/deployments_controller_spec.rb
+++ b/src/spec/controllers/deployments_controller_spec.rb
@@ -65,6 +65,7 @@ describe DeploymentsController do
         @catalog = Factory.create(:catalog)
         @deployable = Factory.create(:deployable, :catalogs => [@catalog])
         Deployment.stub!(:new).and_return(@deployment)
+        Deployment.any_instance.stub(:create_and_launch).and_return(true)
         post :create, :deployable_id => @deployable.id
       end
 
diff --git a/src/spec/models/deployment_spec.rb b/src/spec/models/deployment_spec.rb
index f382eda..4b188e0 100644
--- a/src/spec/models/deployment_spec.rb
+++ b/src/spec/models/deployment_spec.rb
@@ -222,11 +222,11 @@ describe Deployment do
     end
 
     it "should launch instances when launching deployment" do
-      @deployment.save!
       @deployment.instances.should be_empty
 
       Taskomatic.stub!(:create_instance!).and_return(true)
-      @deployment.launch(@user_for_launch)[:errors].should be_empty
+      @deployment.create_and_launch(@user_for_launch)
+      @deployment.errors.should be_empty
       @deployment.instances.count.should == 2
     end
 
@@ -238,7 +238,8 @@ describe Deployment do
       Taskomatic.stub!(:create_dcloud_instance).and_return(true)
       Taskomatic.stub!(:handle_dcloud_error).and_return(true)
       Taskomatic.stub!(:handle_instance_state).and_return(true)
-      @deployment.launch(@user_for_launch)[:errors].should be_empty
+      @deployment.create_and_launch(@user_for_launch)
+      @deployment.errors.should be_empty
       @deployment.reload
       @deployment.instances.count.should == 2
       @deployment.instances[0].provider_account.should == @provider_account1
@@ -246,7 +247,8 @@ describe Deployment do
       @provider_account1.priority = 30
       @provider_account1.save!
       deployment2 = Factory.create(:deployment, :pool_id => @pool.id)
-      deployment2.launch(@user_for_launch)[:errors].should be_empty
+      deployment2.create_and_launch(@user_for_launch)
+      deployment2.errors.should be_empty
       deployment2.reload
       deployment2.instances.count.should == 2
       deployment2.instances[0].provider_account.should == @provider_account2
@@ -264,28 +266,27 @@ describe Deployment do
       Taskomatic.stub!(:create_dcloud_instance).and_return(true)
       Taskomatic.stub!(:handle_dcloud_error).and_return(true)
       Taskomatic.stub!(:handle_instance_state).and_return(true)
-      @deployment.launch(@user_for_launch)[:errors].should be_empty
+      @deployment.create_and_launch(@user_for_launch)
+      @deployment.errors.should be_empty
       @deployment.reload
       @deployment.instances.count.should == 2
       @deployment.instances[0].provider_account.should == @provider_account2
       @deployment.instances[1].provider_account.should == @provider_account2
     end
 
-    it "should set create_failed status for instances if match not found" do
-      @deployment.save!
+    it "should not create deployment with instances if match not found" do
       @deployment.instances.should be_empty
       @deployment.pool.pool_family.provider_accounts.destroy_all
       Taskomatic.stub!(:create_instance!).and_return(true)
-      @deployment.launch(@user_for_launch)
-      @deployment.instances.should_not be_empty
-      @deployment.instances.each {|i| i.state.should == Instance::STATE_CREATE_FAILED}
+      @deployment.create_and_launch(@user_for_launch)
+      @deployment.errors.should_not be_empty
+      lambda { Deployment.find(@deployment.id) }.should raise_error(ActiveRecord::RecordNotFound)
     end
 
     it "should set create_failed status for instances if instance's launch raises an exception" do
-      @deployment.save!
       @deployment.instances.should be_empty
       Taskomatic.stub!(:create_dcloud_instance).and_raise("an exception")
-      @deployment.launch(@user_for_launch)
+      @deployment.create_and_launch(@user_for_launch)
       @deployment.reload
       @deployment.instances.should_not be_empty
       @deployment.instances.each {|i| i.state.should == Instance::STATE_CREATE_FAILED}
-- 
1.7.7.6




More information about the aeolus-devel mailing list