Fixes #15306 - Catches exceptions and rolls back the orchestration

app/models/concerns/orchestration.rb

@@ -8,7 +8,7 @@ module Orchestration
     attr_reader :old
 
     # save handles both creation and update of hosts
-    before_save :on_save
+    around_save :around_save_orchestration
     after_commit :post_commit
     after_destroy :on_destroy
   end
@@ -31,8 +31,16 @@ def register_rebuild(method, pretty_name)
 
   protected
 
-  def on_save
+  def around_save_orchestration
     process :queue
+
+    begin
+      yield
+    rescue ActiveRecord::ActiveRecordError => e
+      Foreman::Logging.exception "Rolling back due to exception during save", e
+      fail_queue queue
+      raise e
+    end
   end
 
   def post_commit
@@ -82,14 +90,30 @@ def record_conflicts
     @record_conflicts ||= []
   end
 
+  def skip_orchestration!
+    @skip_orchestration = true
+  end
+
+  def enable_orchestration!
+    @skip_orchestration = false
+  end
+
+  def skip_orchestration?
+    # The orchestration should be disabled in tests in order to avoid side effects.
+    # If a test needs to enable orchestration, it should be done explicitly by stubbing
+    # this method.
+    return true if Rails.env.test?
+    !!@skip_orchestration
+  end
+
   private
 
   # Handles the actual queue
   # takes care for running the tasks in order
   # if any of them fail, it rollbacks all completed tasks
   # in order not to keep any left overs in our proxies.
   def process(queue_name)
-    return true if Rails.env == "test"
+    return true if skip_orchestration?
 
     # queue is empty - nothing to do.
     q = send(queue_name)
@@ -120,6 +144,12 @@ def process(queue_name)
     return true if q.failed.empty? and q.pending.empty? and q.conflict.empty? and orchestration_errors?
 
     logger.warn "Rolling back due to a problem: #{q.failed + q.conflict}"
+    fail_queue(q)
+
+    rollback
+  end
+
+  def fail_queue(q)
     q.pending.each{ |task| task.status = "canceled" }
 
     # handle errors
@@ -134,8 +164,6 @@ def process(queue_name)
         failure _("Failed to perform rollback on %{task} - %{e}") % { :task => task.name, :e => e }, e
       end
     end
-
-    rollback
   end
 
   def add_conflict(e)

app/models/concerns/orchestration/dhcp.rb

@@ -3,8 +3,9 @@ module Orchestration::DHCP
   include Orchestration::Common
 
   included do
-    after_validation :dhcp_conflict_detected?, :queue_dhcp, :unless => :importing_facts
-    before_destroy :queue_dhcp_destroy, :unless => :importing_facts
+    after_validation :dhcp_conflict_detected?, :unless => :skip_orchestration?
+    after_validation :queue_dhcp
+    before_destroy :queue_dhcp_destroy
     register_rebuild(:rebuild_dhcp, N_('DHCP'))
   end
 

app/models/concerns/orchestration/dns.rb

@@ -3,8 +3,9 @@ module Orchestration::DNS
   include Orchestration::Common
 
   included do
-    after_validation :dns_conflict_detected?, :queue_dns, :unless => :importing_facts
-    before_destroy :queue_dns_destroy, :unless => :importing_facts
+    after_validation :dns_conflict_detected?, :unless => :skip_orchestration?
+    after_validation :queue_dns
+    before_destroy :queue_dns_destroy
     register_rebuild(:rebuild_dns, N_('DNS'))
   end
 

app/models/concerns/orchestration/puppetca.rb

@@ -3,8 +3,9 @@ module Orchestration::Puppetca
 
   included do
     attr_reader :puppetca
-    after_validation :initialize_puppetca, :queue_puppetca, :unless => :importing_facts
-    before_destroy :initialize_puppetca, :queue_puppetca_destroy unless Rails.env == "test"
+    after_validation :initialize_puppetca, :unless => :skip_orchestration?
+    after_validation :queue_puppetca
+    before_destroy :initialize_puppetca, :queue_puppetca_destroy
   end
 
   protected

app/models/concerns/orchestration/realm.rb

@@ -2,8 +2,8 @@ module Orchestration::Realm
   extend ActiveSupport::Concern
 
   included do
-    after_validation  :queue_realm, :unless => :importing_facts
-    before_destroy    :queue_realm_destroy, :unless => :importing_facts
+    after_validation  :queue_realm
+    before_destroy    :queue_realm_destroy
   end
 
   def realm?

app/models/concerns/orchestration/tftp.rb

@@ -2,8 +2,9 @@ module Orchestration::TFTP
   extend ActiveSupport::Concern
 
   included do
-    after_validation :validate_tftp, :queue_tftp, :unless => :importing_facts
-    before_destroy :queue_tftp_destroy, :unless => :importing_facts
+    after_validation :validate_tftp, :unless => :skip_orchestration?
+    after_validation :queue_tftp
+    before_destroy :queue_tftp_destroy
 
     # required for pxe template url helpers
     include Rails.application.routes.url_helpers

app/models/host/base.rb

@@ -76,9 +76,6 @@ def self.taxonomy_conditions
     scope :no_location,     -> { rewhere(:location_id => nil) }
     scope :no_organization, -> { rewhere(:organization_id => nil) }
 
-    # If importing_facts is set to true, all orchestration calls will be skipped
-    attr_accessor :importing_facts
-
     # primary interface is mandatory because of delegated methods so we build it if it's missing
     # similar for provision interface
     # we can't set name attribute until we have primary interface so we don't pass it to super
@@ -108,8 +105,6 @@ def initialize(*args)
 
       super(*args)
 
-      self.importing_facts ||= false
-
       build_required_interfaces
       values_for_primary_interface.each do |name, value|
         self.send "#{name}=", value
@@ -164,7 +159,7 @@ def import_facts(facts)
       importer = FactImporter.importer_for(type).new(self, facts)
       importer.import!
 
-      self.importing_facts = true
+      skip_orchestration!
       save(:validate => false)
       populate_fields_from_facts(facts, type)
       set_taxonomies(facts)
@@ -176,7 +171,7 @@ def import_facts(facts)
       # we should probably send out an alert.
       save(:validate => false)
     ensure
-      self.importing_facts = false
+      enable_orchestration!
     end
 
     def attributes_to_import_from_facts

app/models/nic/managed.rb

@@ -21,7 +21,7 @@ class Managed < Interface
              :operatingsystem, :provisioning_template, :jumpstart?, :build, :build?, :os, :arch,
              :image_build?, :pxe_build?, :pxe_build?, :token, :to_ip_address, :model, :to => :host
     delegate :operatingsystem_id, :hostgroup_id, :environment_id,
-             :overwrite?, :importing_facts, :to => :host, :allow_nil => true
+             :overwrite?, :skip_orchestration!, :to => :host, :allow_nil => true
 
     attr_exportable :ip, :mac, :name, :attrs, :virtual, :link, :identifier, :managed, :primary, :provision, :subnet, :subnet6,
       :tag => ->(nic) { nic.tag if nic.virtual? },

db/migrate/20150612135546_create_host_status.rb

@@ -13,10 +13,10 @@ def up
     success = true
     Host.includes(:host_statuses, :last_report_object).find_each do |host|
       begin
-        host.importing_facts = true # disable orchestration
+        host.skip_orchestration! # disable orchestration
         success &= update_statuses(host)
       ensure
-        host.importing_facts = false
+        host.enable_orchestration!
       end
     end
     say "some host status could not be saved, please see the log for more details" unless success

test/unit/orchestration_test.rb

@@ -34,7 +34,7 @@ def test_host_should_have_queue
   end
 
   test "test host can call protected queue methods" do
-    class Host::Test < Host::Base
+    class Host::Test1 < Host::Base
       include Orchestration
       def test_execute(method)
         execute({:action => [self, method]})
@@ -44,13 +44,51 @@ def test_execute(method)
 
       def setTest; true; end
     end
-    h = Host::Test.new
+    h = Host::Test1.new
     assert h.test_execute(:setTest)
     assert_raise Foreman::Exception do
       h.test_execute(:noSuchTest)
     end
   end
 
+  test "test host compensates queue if active record not saved" do
+    class Host::Test2 < Host::Base
+      include Orchestration
+
+      attr_accessor :progress_report_id
+
+      after_validation :queue_test
+
+      def lookup_value_match
+        "fqdn=zzz"
+      end
+
+      def queue_test
+        queue.create(
+          :name => 'test action',
+          :priority => 1,
+          :action => [self, :setTestFlags]
+        )
+      end
+
+      def setTestFlags
+        true
+      end
+    end
+
+    h = Host::Test2.new(:name => 'test1')
+    h.stubs(:skip_orchestration?).returns(false)
+    h.stubs(:update_cache).returns(true)
+    h.stubs(:_create_record).raises(ActiveRecord::InvalidForeignKey, 'Fake foreign key exception')
+
+    # rollback should be called
+    h.expects(:delTestFlags).returns(true)
+
+    assert_raise ActiveRecord::InvalidForeignKey do
+      h.save!
+    end
+  end
+
   test "parameters can be passed to queue methods" do
     class Host::Test < Host::Base
       include Orchestration