[PATCH conductor] Deployable deadlock detecting fix
Imre Farkas
ifarkas at redhat.com
Wed Sep 12 12:07:32 UTC 2012
On 09/12/2012 12:28 PM, jprovazn at redhat.com wrote:
> From: Jan Provaznik <jprovazn at redhat.com>
>
> When Configserver launches services on each VM, it checks if
> all param references are satisfied so generally something like
> this should cause a deadlock because of cyclic reference:
> Assembly A, param P1 references Assembly B, param P2
> Assembly B, param P1 references Assembly A, param P2
>
> But the trick is that the above deadlock is allowed if
> referenced param is 'OS' type (if it's value is not output of
> some script). So cyclic references can be allowed in some cases
> depending on type of referenced parameter.
>
> But problem is that type of referenced parameter can't be easily resolved
> based only on deployable XML file so only solution for Conductor for now
> is to enable inter-assembly cyclic references and check only cyclic references inside
> one assembly.
> ---
> src/app/models/deployable.rb | 2 +-
> src/app/util/deployable_xml.rb | 45 +++++++++++++++-----------------
> src/spec/factories/deployable.rb | 53 +++-----------------------------------
> src/spec/models/deployable_spec.rb | 27 +++++++------------
> 4 files changed, 35 insertions(+), 92 deletions(-)
>
> diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
> index 374b4e3..811a513 100644
> --- a/src/app/models/deployable.rb
> +++ b/src/app/models/deployable.rb
> @@ -98,7 +98,7 @@ class Deployable < ActiveRecord::Base
> errors.add(:xml,
> I18n.t('catalog_entries.flash.warning.not_valid_not_existing_param',
> locale_params))
> - elsif reference[:to_service]
> + elsif reference[:reference][:service]
> errors.add(:xml,
> I18n.t('catalog_entries.flash.warning.not_valid_not_existing_service_reference',
> locale_params))
> diff --git a/src/app/util/deployable_xml.rb b/src/app/util/deployable_xml.rb
> index e057771..837041b 100644
> --- a/src/app/util/deployable_xml.rb
> +++ b/src/app/util/deployable_xml.rb
> @@ -184,12 +184,6 @@ class AssemblyXML
> :service => service.name,
> :references => service.references}
> end
> - # whole assembly is added as a node too because a inter-assembly references
> - # reference whole assembly, not particular service
> - nodes << {:assembly => name,
> - :service => nil,
> - :references => nodes.map {|n|
> - n[:references]}.flatten}
> end
>
> private
> @@ -343,28 +337,31 @@ class DeployableDependencyGraph
>
> invalid_refs = []
> dependency_nodes.each do |node|
> - # skip "whole assembly" nodes
> - next unless node[:service]
> -
> node[:references].each do |ref|
> - if ref[:not_existing_ref]
> - # in this case, the referenced assembly/service doesn't exist
> - # at all
> - invalid_refs << {:assembly => node[:assembly],
> - :service => node[:service],
> - :reference => ref}
> - else
> - # in this case, the referenced assembly/service exists, but the
> - # referenced parameter is not listed in <returns> tag of the assembly
> + no_return_param = false
> +
> + if ref[:service].nil?
> assembly = @assemblies.find {|a| a.name == ref[:assembly]}
> - next unless assembly # this shouldn't be needed
> - unless assembly.output_parameters.include?(ref[:param])
> - invalid_refs << {:assembly => node[:assembly],
> - :service => node[:service],
> - :no_return_param => true,
> - :reference => ref}
> + if assembly
> + if assembly.output_parameters.include?(ref[:param])
> + # if referenced assembly exists and returns referenced param, then
> + # it's valid
> + next
> + else
> + no_return_param = true
> + end
> end
> + else
> + # if a param references another service, then we check if
> + # this service exists or not. For current version of config server
> + # only services from the same assembly can be referenced
> + next unless ref[:not_existing_ref]
> end
> +
> + invalid_refs << {:assembly => node[:assembly],
> + :service => node[:service],
> + :no_return_param => no_return_param,
> + :reference => ref}
> end
> end
> invalid_refs
> diff --git a/src/spec/factories/deployable.rb b/src/spec/factories/deployable.rb
> index 8a4f3ba..b279159 100644
> --- a/src/spec/factories/deployable.rb
> +++ b/src/spec/factories/deployable.rb
> @@ -73,54 +73,6 @@ FactoryGirl.define do
> </deployable>"
> end
>
> - factory :deployable_with_cyclic_assembly_references, :parent => :deployable do
> - xml "<?xml version=\"1.0\"?>
> - <deployable version='1.0' name='deployable'>
> - <assemblies>
> - <assembly name='assembly1' hwp='front_hwp1'>
> - <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
> - <services>
> - <service name='service1'>
> - <executable url='executable_url'/>
> - <parameters>
> - <parameter name='param1'>
> - <reference assembly='assembly2' parameter='hostname'/>
> - </parameter>
> - </parameters>
> - </service>
> - </services>
> - </assembly>
> - <assembly name='assembly2' hwp='front_hwp1'>
> - <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
> - <services>
> - <service name='service1'>
> - <executable url='executable_url'/>
> - <parameters>
> - <parameter name='param1'>
> - <reference assembly='assembly3' parameter='hostname'/>
> - </parameter>
> - </parameters>
> - </service>
> - </services>
> - </assembly>
> - <assembly name='assembly3' hwp='front_hwp1'>
> - <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
> - <services>
> - <service name='service1'>
> - <executable url='executable_url'/>
> - <parameters>
> - <parameter name='param1'>
> - <reference assembly='assembly1' parameter='hostname'/>
> - </parameter>
> - </parameters>
> - </service>
> - </services>
> - </assembly>
> - </assemblies>
> - </deployable>"
> - end
> -
> -
> factory :deployable_with_cyclic_service_references, :parent => :deployable do
> xml "<?xml version=\"1.0\"?>
> <deployable version='1.0' name='deployable'>
> @@ -161,7 +113,10 @@ FactoryGirl.define do
> <executable url='executable_url'/>
> <parameters>
> <parameter name='param1'>
> - <reference assembly='assembly2' service='service2'/>
> + <reference assembly='assembly1' service='service2'/>
> + </parameter>
> + <parameter name='param2'>
> + <reference assembly='assembly2' parameter='param2'/>
> </parameter>
> </parameters>
> </service>
> diff --git a/src/spec/models/deployable_spec.rb b/src/spec/models/deployable_spec.rb
> index 4aec099..0deae4c 100644
> --- a/src/spec/models/deployable_spec.rb
> +++ b/src/spec/models/deployable_spec.rb
> @@ -98,23 +98,6 @@ describe Deployable do
> end
> end
>
> - describe "validation of deployable_xml with cyclic assembly references" do
> - before :each do
> - @deployable = FactoryGirl.build(:deployable_with_cyclic_assembly_references)
> - @cycles = DeployableXML.new(@deployable.xml).dependency_graph.cycles
> - end
> -
> - it_behaves_like "deployable XML with cyclic reference"
> -
> - it "should detect cycle between multiple assmebliess" do
> - cycle = @cycles.first
> - cycle.length.should == 3
> - cycle.find {|n| n[:assembly] == 'assembly1' && n[:service] == 'service1'}
> - cycle.find {|n| n[:assembly] == 'assembly2' && n[:service] == 'service1'}
> - cycle.find {|n| n[:assembly] == 'assembly3' && n[:service] == 'service1'}
> - end
> - end
> -
> describe "validation of deployable_xml with not existing references" do
> before :each do
> @deployable = FactoryGirl.build(:deployable_with_not_existing_references)
> @@ -136,9 +119,17 @@ describe Deployable do
> it "should detect reference to not existing service" do
> @invalid_refs.find {|r| r[:assembly] == 'assembly1' &&
> r[:service] == 'service1' &&
> - r[:reference][:assembly] == 'assembly2' &&
> + r[:reference][:assembly] == 'assembly1' &&
> r[:reference][:service] == 'service2'}.should_not be_nil
> end
> +
> + it "should detect reference to not existing parameter" do
> + @invalid_refs.find {|r| r[:assembly] == 'assembly1' &&
> + r[:service] == 'service1' &&
> + r[:no_return_param] &&
> + r[:reference][:assembly] == 'assembly2' &&
> + r[:reference][:param] == 'param2'}.should_not be_nil
> + end
> end
>
> it "should manage permissions on catalog changes" do
>
ACK
More information about the aeolus-devel
mailing list