[PATCH conductor] Deployable deadlock detecting fix

jprovazn at redhat.com jprovazn at redhat.com
Wed Sep 12 10:28:48 UTC 2012


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
-- 
1.7.11.4




More information about the aeolus-devel mailing list