From: Jan Provaznik <jprovazn(a)redhat.com>
This patch tries to partially cleanup huge deployment launch method,
the behavior of this method should be unchanged.
- Instance + InstanceParameter objects creation is moved to separate method
- common_provider_accounts_for replaced with create_instances_with_params method:
this method returns only first possible match (and keeps account's priority
ordering by using ascending_by_priority scope)
- instance's launch block (including config setup) moved to separate Instance#launch
method
- added :dependent => :destroy for events association, which I think is missing by
mistake
---
src/app/models/deployment.rb | 203 +++++++++++++----------------------
src/app/models/instance.rb | 37 ++++++-
src/app/models/provider_account.rb | 5 +-
src/spec/models/deployment_spec.rb | 23 ++--
src/spec/models/instance_spec.rb | 2 +-
5 files changed, 124 insertions(+), 146 deletions(-)
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index 989d60f..f78a408 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -54,7 +54,7 @@ class Deployment < ActiveRecord::Base
:order => "permissions.id ASC"
belongs_to :owner, :class_name => "User", :foreign_key =>
"owner_id"
- has_many :events, :as => :source
+ has_many :events, :as => :source, :dependent => :destroy
has_many :provider_accounts, :through => :instances
@@ -204,103 +204,37 @@ class Deployment < ActiveRecord::Base
# TODO: need to be able to handle deployable-level errors
#
status = { :errors => {}, :successes => {} }
- assembly_instances = {}
- deployable_xml.assemblies.each do |assembly|
- begin
- hw_profile = permissioned_frontend_hwprofile(user, assembly.hwp)
- raise "Hardware Profile #{assembly.hwp} not found." 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
- end
- assembly_instances[assembly.name] = instance
- end
- rescue
- logger.error $!.message
- logger.error $!.backtrace.join("\n ")
- status[:errors][assembly.name] = $!.message.to_s.split("\n").first
- end
- end
+ status[:errors] = create_instances_with_params(user)
+ all_inst_match, account, errors = find_match_with_common_account
- account_matches, errors = common_provider_accounts_for(assembly_instances.values)
- if account_matches.empty?
+ unless all_inst_match
status[:errors][name] = errors
return status
end
- # grab the account with the best priority that can launch this deployment
- account = account_matches.keys.sort do |a,b|
- if a.priority.nil? and b.priority.nil?
- 0
- elsif a.priority.nil?
- 1
- elsif b.priority.nil?
- -1
- else
- a.priority <=> b.priority
- end
- end.first
- instances_matches = account_matches[account]
if deployable_xml.requires_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)
+ instance_configs = ConfigServerUtil.instance_configs(self, instances,
config_server)
+ else
+ config_server = nil
+ instance_configs = {}
end
- assembly_instances.each do |assembly_name, instance|
- match = instances_matches[instance]
- config = instance_configs[instance.uuid] if deployable_xml.requires_config_server?
+ instances.each do |instance|
+ match = all_inst_match.find{|m| m.instance.id == instance.id}
begin
- if deployable_xml.requires_config_server?
- instance.user_data = Instance.generate_user_data(instance, config_server)
- 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})
- if deployable_xml.requires_config_server?
- begin
- config_server.send_config(config)
- rescue Errno::ECONNREFUSED
- raise I18n.t 'deployments.errors.config_server_connection'
- end
- end
- Taskomatic.create_instance(task, match)
- if task.state == Task::STATE_FAILED
- status[:errors][assembly_name] = 'failed'
+ if instance.launch(match, user, config_server, instance_configs[instance.uuid])
+ status[:successes][instance.name] = 'launched'
else
- status[:successes][assembly_name] = 'launched'
+ status[:errors][instance.name] = 'failed'
end
rescue
logger.error $!.message
logger.error $!.backtrace.join("\n ")
- status[:errors][assembly_name] = $!.message.to_s.split("\n").first
+ status[:errors][instance.name] = $!.message.to_s.split("\n").first
end
end
status
@@ -352,7 +286,6 @@ class Deployment < ActiveRecord::Base
# if a match is found
def check_assemblies_matches(user)
errs = []
- instances = []
begin
deployable_xml.assemblies.each do |assembly|
hw_profile = permissioned_frontend_hwprofile(user, assembly.hwp)
@@ -376,8 +309,8 @@ class Deployment < ActiveRecord::Base
end
end
- account_matches, errors = common_provider_accounts_for(instances)
- if account_matches.empty? and not errors.empty?
+ match, account, errors = find_match_with_common_account
+ if match.nil? and not errors.empty?
raise
Aeolus::Conductor::MultiError::UnlaunchableAssembly.new(I18n.t('deployments.flash.error.not_launched'),
errors)
end
@@ -547,55 +480,29 @@ 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
+ # Find account (w/ highest priority) where all instances can be launched.
+ # 1. find all matches for each instance
+ # 2. for each account (ordered by priority) try to find match for each
+ # instance
+ def find_match_with_common_account
errors = []
- instance_matches = {}
- instances.each do |instance|
+ all_matches = instances.map do |instance|
m, e = instance.matches
- instance_matches[instance] = m if m and not m.empty?
- errors << e
+ errors = e.map {|e| "#{instance.name}: #{e}"}
+ m
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)
+ pool.pool_family.provider_accounts.ascending_by_priority.each do |account|
+ matches_by_account = all_matches.map do |m|
+ m.find {|m| m.provider_account.id == account.id}
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.name)
- rejected = true
- end
+ next if matches_by_account.include?(nil)
+ unless account.quota.can_start? instances
+ errors << I18n.t('instances.errors.provider_account_quota_too_low',
:match_provider_account => account)
+ next
end
- rejected
- end if not account_matches.empty?
-
- [account_matches, errors]
+ return [matches_by_account, account, errors] unless
matches_by_account.include?(nil)
+ end
+ [nil, nil, errors]
end
def generate_uuid
@@ -610,4 +517,46 @@ class Deployment < ActiveRecord::Base
self[:pool_family_id] = pool.pool_family_id
end
+ def create_instances_with_params(user)
+ errors = {}
+ deployable_xml.assemblies.each do |assembly|
+ begin
+ hw_profile = permissioned_frontend_hwprofile(user, assembly.hwp)
+ raise "Hardware Profile #{assembly.hwp} not found." 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
+ end
+ self.instances << instance
+ end
+ rescue
+ logger.error $!.message
+ logger.error $!.backtrace.join("\n ")
+ errors[assembly.name] = $!.message.to_s.split("\n").first
+ end
+ end
+ errors
+ end
end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index d2c5f8e..e67532f 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -317,8 +317,19 @@ class Instance < ActiveRecord::Base
(0..(rand(10) + 40)).map { OAUTH_SECRET_SEED[rand(OAUTH_SECRET_SEED.length)] }.join
end
- def self.generate_user_data(instance, config_server)
-
["#{USER_DATA_VERSION}|#{config_server.endpoint}|#{instance.uuid}|#{instance.secret}"].pack("m0").delete("\n")
+ def generate_user_data(config_server)
+
["#{USER_DATA_VERSION}|#{config_server.endpoint}|#{uuid}|#{secret}"].pack("m0").delete("\n")
+ end
+
+ def add_instance_config!(config_server, config)
+ user_data = generate_user_data(config_server)
+ instance_config_xml = config.to_s
+ save!
+ begin
+ config_server.send_config(config)
+ rescue Errno::ECONNREFUSED
+ raise I18n.t 'deployments.errors.config_server_connection'
+ end
end
def restartable?
@@ -347,22 +358,25 @@ class Instance < ActiveRecord::Base
end
class Match
- attr_reader :pool_family, :provider_account, :hwp, :provider_image, :realm
+ attr_reader :pool_family, :provider_account, :hwp, :provider_image, :realm,
:instance
- def initialize(pool_family, provider_account, hwp, provider_image, realm)
+ def initialize(pool_family, provider_account, hwp, provider_image, realm, instance)
@pool_family = pool_family
@provider_account = provider_account
@hwp = hwp
@provider_image = provider_image
@realm = realm
+ @instance = instance
end
def ==(other)
+ return self.nil? && other.nil? if (self.nil? || other.nil?)
self.pool_family == other.pool_family &&
self.provider_account == other.provider_account &&
self.hwp == other.hwp &&
self.provider_image == other.provider_image &&
self.realm == other.realm
+ self.instance == other.instance
end
end
@@ -393,13 +407,26 @@ class Instance < ActiveRecord::Base
return [[], errors] unless errors.empty?
matched = []
- pool.pool_family.provider_accounts.each do |account|
+ pool.pool_family.provider_accounts.ascending_by_priority.each do |account|
account.instance_matches(self, matched, errors)
end
[matched, errors]
end
+ def launch(match, user, config_server, config)
+ if config_server
+ add_instance_config!(config_server, config)
+ end
+ # create a taskomatic task
+ task = InstanceTask.create!({:user => user,
+ :task_target => self,
+ :action => InstanceTask::ACTION_CREATE})
+ Taskomatic.create_instance(task, match)
+ task.state != Task::STATE_FAILED
+ end
+
+
def self.csv_export(instances)
csv_string = FasterCSV.generate(:col_sep => ";", :row_sep =>
"\r\n") do |csv|
event_attributes = Event.new.attributes.keys.reject {|key| key if key ==
"created_at" || key == "updated_at"}
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index 05fb622..9647dd5 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -73,6 +73,7 @@ class ProviderAccount < ActiveRecord::Base
before_destroy :destroyable?
scope :enabled, lambda { where(:provider_id => Provider.enabled) }
+ scope :ascending_by_priority, :order => 'provider_accounts.priority ASC'
# We set credentials hash as protected so that it is not set during mass assign on new
# This is to avoid the scenario where the credentials are set before provider which
@@ -370,11 +371,11 @@ class ProviderAccount < ActiveRecord::Base
# backend realm which is available and is accessible for this
# provider account
if (brealm_target.target_realm.nil? || (brealm_target.target_realm.available
&& realms.include?(brealm_target.target_realm)))
- matched << Instance::Match.new(instance.pool.pool_family, self, hwp,
pi, brealm_target.target_realm)
+ matched << Instance::Match.new(instance.pool.pool_family, self, hwp,
pi, brealm_target.target_realm, instance)
end
end
else
- matched << Instance::Match.new(instance.pool.pool_family, self, hwp, pi,
nil)
+ matched << Instance::Match.new(instance.pool.pool_family, self, hwp, pi,
nil, instance)
end
end
end
diff --git a/src/spec/models/deployment_spec.rb b/src/spec/models/deployment_spec.rb
index 0feecfc..b390ca7 100644
--- a/src/spec/models/deployment_spec.rb
+++ b/src/spec/models/deployment_spec.rb
@@ -412,24 +412,25 @@ describe Deployment do
account1 = FactoryGirl.create(:mock_provider_account, :label =>
"test_account1")
account2 = FactoryGirl.create(:mock_provider_account, :label =>
"test_account2")
account3 = FactoryGirl.create(:mock_provider_account, :label =>
"test_account3")
- possible1 = Instance::Match.new(nil,account1,nil,nil,nil)
- possible2 = Instance::Match.new(nil,account2,nil,nil,nil)
- possible3 = Instance::Match.new(nil,account2,nil,nil,nil)
- possible4 = Instance::Match.new(nil,account3,nil,nil,nil)
- possible5 = Instance::Match.new(nil,account2,nil,nil,nil)
+ @deployment.pool.pool_family.provider_accounts += [account1, account2, account3]
+ possible1 = Instance::Match.new(nil,account1,nil,nil,nil, nil)
+ possible2 = Instance::Match.new(nil,account2,nil,nil,nil, nil)
+ possible3 = Instance::Match.new(nil,account2,nil,nil,nil, nil)
+ possible4 = Instance::Match.new(nil,account3,nil,nil,nil, nil)
+ possible5 = Instance::Match.new(nil,account2,nil,nil,nil, nil)
# not gonna test the individual instance "machtes" logic again
# just stub out the behavior
- instance1 = Factory.build(:instance)
+ @deployment.instances << instance1 = Factory.build(:instance)
instance1.stub!(:matches).and_return([[possible1, possible2], []])
- instance2 = Factory.build(:instance)
+ @deployment.instances << instance2 = Factory.build(:instance)
instance2.stub!(:matches).and_return([[possible3, possible4], []])
- instance3 = Factory.build(:instance)
+ @deployment.instances << instance3 = Factory.build(:instance)
instance3.stub!(:matches).and_return([[possible5], []])
instances = [instance1, instance2, instance3]
- matches, errors = @deployment.send(:common_provider_accounts_for, instances)
- matches.should_not be_empty
- matches.keys.first.should eql(account2)
+ match, account, errors = @deployment.send(:find_match_with_common_account)
+ match.should_not be_nil
+ account.should eql(account2)
end
end
diff --git a/src/spec/models/instance_spec.rb b/src/spec/models/instance_spec.rb
index 56b3663..edcc68d 100644
--- a/src/spec/models/instance_spec.rb
+++ b/src/spec/models/instance_spec.rb
@@ -203,7 +203,7 @@ describe Instance do
quota.running_instances.should == 1
quota.should be_reached
# I'm not sure why this line is required, but it is:
- @instance.pool.pool_family.provider_accounts.first.quota.reload
+ @instance.reload
@instance.matches.first.should be_empty
end
--
1.7.7.6