[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