[PATCH Conductor] Allow conductor to launch multiple unique provider images

gblomqui at redhat.com gblomqui at redhat.com
Wed Dec 14 20:24:43 UTC 2011


From: Greg Blomquist <gblomqui at redhat.com>

Conductor can now launch a deployable with multiple images in a single
deployable.  Previously conductor was only able to launch deployables with the
same image because of a bug in the instance <-> provider account matching logic.

see https://bugzilla.redhat.com/show_bug.cgi?id=767759 for more details.
---
 src/app/models/deployment.rb                       |  104 +++++++++++++++-----
 src/app/models/instance.rb                         |   27 -----
 src/app/models/instance_observer.rb                |    2 +-
 src/app/util/taskomatic.rb                         |   13 +---
 .../controllers/deployments_controller_spec.rb     |    4 +-
 5 files changed, 82 insertions(+), 68 deletions(-)

diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index 1a799b8..8c10913 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -173,26 +173,23 @@ class Deployment < ActiveRecord::Base
     @launch_parameters = launch_parameters
   end
 
-  def launch(user, config_values = nil)
-    # first, create all the instance objects,
-    # if a config server is being used
-    #     find if there is at least a single account where they can all launch,
-    # then generate the instance configs for the instances,
-    # if a using a config server
-    #     then, for each instance send the instance configs to the config server,
-    # and launch the instances
+  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,
+    #   then send the instance configs to the config server,
+    # finally, launch the instances
     #
     # TODO: need to be able to handle deployable-level errors
     #
     status = { :errors => {}, :successes => {} }
     assembly_instances = {}
     deployable_xml.assemblies.each do |assembly|
-      # TODO: for now we try to start all instances even if some of them fails
       begin
-        task = nil
+        hw_profile = permissioned_frontend_hwprofile(user, assembly.hwp)
+        raise "Hardware Profile #{assembly.hwp} not found." unless hw_profile
         Instance.transaction do
-          hw_profile = permissioned_frontend_hwprofile(user, assembly.hwp)
-          raise "Hardware Profile #{assembly.hwp} not found." unless hw_profile
           instance = Instance.create!(
             :deployment => self,
             :name => "#{name}/#{assembly.name}",
@@ -205,7 +202,6 @@ class Deployment < ActiveRecord::Base
             :owner => user,
             :hardware_profile => hw_profile
           )
-          # store the assembly's parameters
           assembly.services.each do |service|
             service.parameters.each do |parameter|
               if not parameter.reference?
@@ -219,7 +215,6 @@ class Deployment < ActiveRecord::Base
               end
             end
           end
-
           assembly_instances[assembly.name] = instance
         end
       rescue
@@ -228,21 +223,25 @@ class Deployment < ActiveRecord::Base
         status[:errors][assembly.name] = $!.message
       end
     end
-    # figure out which config server to use
-    # and, generate the instance configurations for the instances
+
+    account_matches, errors = common_provider_accounts_for(assembly_instances.values)
+    if account_matches.empty?
+      status[:errors][name] = errors
+      return status
+    end
+    account = account_matches.keys.first
+    instances_matches = account_matches[account]
+
     if deployable_xml.requires_config_server?
-      matches, errors = Instance.matches(assembly_instances.values)
-      if matches.empty?
-        #TODO:need to have a way to show the errors in a meaningful way
-        status[:errors][name] = errors
-        return status
-      end
-      found = matches.first
-      config_server = found.provider_account.config_server
+      # the instance configurations need to be generated from the entire set of
+      # 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, assembly_instances.values, config_server)
     end
-    # now actually do the launch
+
     assembly_instances.each do |assembly_name, instance|
+      match = instances_matches[instance]
       config = instance_configs[instance.uuid] if deployable_xml.requires_config_server?
       begin
         if deployable_xml.requires_config_server?
@@ -250,7 +249,7 @@ class Deployment < ActiveRecord::Base
           instance.instance_config_xml = config.to_s
           instance.save!
         end
-
+        # create a taskomatic task
         task = InstanceTask.create!({:user        => user,
                                      :task_target => instance,
                                      :action      => InstanceTask::ACTION_CREATE})
@@ -261,7 +260,7 @@ class Deployment < ActiveRecord::Base
             raise I18n.t 'deployments.errors.config_server_connection'
           end
         end
-        Taskomatic.create_instance(task)
+        Taskomatic.create_instance(task, match)
         if task.state == Task::STATE_FAILED
           status[:errors][assembly_name] = 'failed'
         else
@@ -468,6 +467,57 @@ class Deployment < ActiveRecord::Base
     end
   end
 
+  # Find the provider accounts where all the given instances can be launched
+  # Returns a tuple of the form:
+  #   [{account => {instance => match, instance => match},
+  #     account => {instance => match, instance => match}},
+  #    errors]
+  def common_provider_accounts_for(instances)
+    matches = nil
+    errors = []
+    instance_matches = {}
+    instances.each do |instance|
+      m, e = instance.matches
+      instance_matches[instance] = m if m and not m.empty?
+      errors << e
+    end
+    # this series of map-reductions takes the input:
+    #   instance_matches
+    #     {i1 => [m1, m2, m3], i2 => [m4, m5, m6]}
+    # and translates it into the form:
+    #     {a1 => {i1 => m1, i2 => m4},
+    #      a2 => {i1 => m2, i2 => m5},
+    #      a3 => {i1 => m3, i2 => m6}}
+    # where iN is an instance, mN is a match, and aN is an account
+    account_matches = instance_matches.map do |instance, matches|
+      matches.map do |match|
+        {match.provider_account => {instance => match}}
+      end.reduce :merge
+    end.reduce do |account_map, instance_map|
+      account_map.merge!(instance_map) do |key, v1, v2|
+        v1.merge(v2)
+      end
+    end || []
+
+    # reject any accounts that cannot launch the entire deployment
+    account_matches.reject! do |account, instance_map|
+      # implies that one or more instance does not have a corresponding image
+      # pushed to this account
+      rejected = instance_map.size < instances.size
+
+      # also check that the account's quota can handle all the instances
+      if not rejected
+        if not account.quota.can_start? instances
+          errors << I18n.t('instances.errors.provider_account_quota_too_low', :match_provider_account => account)
+          rejected = true
+        end
+      end
+      rejected
+    end if not account_matches.empty?
+
+    [account_matches, errors]
+  end
+
   def generate_uuid
     self[:uuid] = UUIDTools::UUID.timestamp_create.to_s
   end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index fec47d1..d0e0f99 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -431,33 +431,6 @@ class Instance < ActiveRecord::Base
     not deployment.instances.deployed.any? {|i| i != self}
   end
 
-  # find the list of possibles that will accommodate all of the instances
-  def self.matches(instances)
-    matches = nil
-    errors = []
-    instances.each do |instance|
-      m, e = instance.matches
-      if matches.nil?
-        matches = m.dup
-      else
-        matches.delete_if {|match| not m.include?(match) }
-      end
-      errors << e
-    end
-    # For now, this only checks the account's quota to see whether all the
-    # instances can launch there
-    # TODO:  Determine if there's more to check here
-    matches.reject! do |match|
-      rejected = false
-      if !match.provider_account.quota.can_start? instances
-        errors << I18n.t('instances.errors.provider_account_quota_too_low', :match_provider_account => match.provider_account)
-        rejected = true
-      end
-      rejected
-    end
-    [matches, errors]
-  end
-
   def stop
     do_operation('stop')
   end
diff --git a/src/app/models/instance_observer.rb b/src/app/models/instance_observer.rb
index 9d13fb0..c609538 100644
--- a/src/app/models/instance_observer.rb
+++ b/src/app/models/instance_observer.rb
@@ -57,7 +57,7 @@ class InstanceObserver < ActiveRecord::Observer
     [provider_account, pool_family, pool, user].each do |parent|
       if parent
         # Since pool and pool_family are related, updating one can cause the other to become stale
-        parent.reload if parent.class == Pool
+        parent.reload if [Pool, ProviderAccount].include? parent.class
         quota = parent.quota
         if quota
           if !RUNNING_STATES.include?(state_from) && RUNNING_STATES.include?(state_to)
diff --git a/src/app/util/taskomatic.rb b/src/app/util/taskomatic.rb
index 9750873..84c31d3 100644
--- a/src/app/util/taskomatic.rb
+++ b/src/app/util/taskomatic.rb
@@ -16,10 +16,8 @@
 
 module Taskomatic
 
-  def self.create_instance(task)
+  def self.create_instance(task, match)
     begin
-      match = matches(task.instance).first
-
       task.state = Task::STATE_PENDING
       task.save!
 
@@ -158,13 +156,4 @@ module Taskomatic
     client_args.merge!({:user_data => instance.user_data}) if instance.user_data.present?
     client.create_instance(client_args)
   end
-
-  def self.matches(instance)
-    matched, errors = instance.matches
-    if matched.empty?
-      raise "Could not find a matching backend provider, errors: #{errors.join(', ')}"
-    end
-    matched
-  end
-
 end
diff --git a/src/spec/controllers/deployments_controller_spec.rb b/src/spec/controllers/deployments_controller_spec.rb
index 4b1c4df..9e1f2b7 100644
--- a/src/spec/controllers/deployments_controller_spec.rb
+++ b/src/spec/controllers/deployments_controller_spec.rb
@@ -59,13 +59,15 @@ describe DeploymentsController do
 
     describe "#create" do
       before do
+        Factory.create(:front_hwp1)
+        Factory.create(:front_hwp2)
         @deployment = Factory.build(:deployment)
         Deployment.stub!(:new).and_return(@deployment)
         post :create
       end
 
       it { response.should be_success }
-      it { ActiveSupport::JSON.decode(response.body)["name"].should == @deployment.name }
+      it { json = ActiveSupport::JSON.decode(response.body)["name"].should == @deployment.name }
     end
 
     describe "#destroy" do
-- 
1.7.7.4




More information about the aeolus-devel mailing list