On 12/14/2011 09:24 PM, gblomqui(a)redhat.com wrote:
> From: Greg Blomquist<gblomqui(a)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
Hi Greg,
2 RSpec tests are failing for me:
rspec ./spec/models/instance_spec.rb:303 # Instance should match only
the intersecting provider accounts for all instances
rspec ./spec/models/deployment_spec.rb:183 # Deployment using image from
iwhd should match provider accounts according to priority when launching
deployment
Apart from that it works for me. ACK.
Thomas
I cleaned up this patch and rebased on 1.0-staging. All tests are
passing now.
Pushing to 1.0-staging.
Thanks Thomas!
---
Greg