Avoid domain undefine on configuration update (#1496)

Calling undefine on a domain and recreating it can result in some edge
case errors where if the current capabilities of libvirt have been
reduced, it may not be possible to restore the old definition.

Instead switch to calling `domain_define` with the new definition and
check that the resulting libvirt domain definition has been updated in
the expected manner, otherwise report an error to the user.

Fixes: #949
Relates-to: #1329
Relates-to: #1027
Relates-to: #1371

lib/vagrant-libvirt/action/start_domain.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require 'diffy'
 require 'log4r'
 require 'rexml/document'
 
@@ -14,8 +15,6 @@ def initialize(app, _env)
         end
 
         def call(env)
-          env[:ui].info(I18n.t('vagrant_libvirt.starting_domain'))
-
           domain = env[:machine].provider.driver.connection.servers.get(env[:machine].id.to_s)
           raise Errors::NoDomainError if domain.nil?
           config = env[:machine].provider_config
@@ -33,6 +32,7 @@ def call(env)
                 libvirt_domain.memory = libvirt_domain.max_memory
               end
             end
+
             begin
               # XML definition manipulation
               descr = libvirt_domain.xml_desc(1)
@@ -248,7 +248,7 @@ def call(env)
               # TPM
               if [config.tpm_path, config.tpm_version].any?
                 if config.tpm_path
-                  raise Errors::FogCreateServerError, 'The TPM Path must be fully qualified' unless config.tpm_path[0].chr == '/'
+                  raise Errors::UpdateServerError, 'The TPM Path must be fully qualified' unless config.tpm_path[0].chr == '/'
                 end
 
                 # just build the tpm element every time
@@ -394,7 +394,6 @@ def call(env)
                 loader.parent.delete_element(loader)
               end
 
-              undefine_flags = 0
               nvram = REXML::XPath.first(xml_descr, '/domain/os/nvram')
               if config.nvram
                 if nvram.nil?
@@ -407,35 +406,65 @@ def call(env)
                     descr_changed = true
                     nvram.text = config.nvram
                   end
-                  undefine_flags |= ProviderLibvirt::Util::DomainFlags::VIR_DOMAIN_UNDEFINE_KEEP_NVRAM
                 end
               elsif !nvram.nil?
                 descr_changed = true
-                undefine_flags |= ProviderLibvirt::Util::DomainFlags::VIR_DOMAIN_UNDEFINE_NVRAM
                 nvram.parent.delete_element(nvram)
               end
 
               # Apply
               if descr_changed
+                env[:ui].info(I18n.t('vagrant_libvirt.updating_domain'))
+                new_xml = String.new
+                xml_descr.write(new_xml)
+                begin
+                  # providing XML for the same name and UUID will update the existing domain
+                  libvirt_domain = env[:machine].provider.driver.connection.define_domain(new_xml)
+                rescue ::Libvirt::Error => e
+                  raise Errors::UpdateServerError, error_message: e.message
+                end
+
                 begin
-                  libvirt_domain.undefine(undefine_flags)
-                  new_descr = String.new
-                  xml_descr.write new_descr
-                  env[:machine].provider.driver.connection.servers.create(xml: new_descr)
-                rescue Fog::Errors::Error => e
-                  env[:machine].provider.driver.connection.servers.create(xml: descr)
-                  raise Errors::FogCreateServerError, error_message: e.message
+                  proposed = Nokogiri::XML(new_xml, &:noblanks)
+
+                  # This normalizes the attribute order to be consistent across both XML docs to eliminate differences
+                  # for subsequent comparison by diffy
+                  updated_xml_descr = REXML::Document.new(libvirt_domain.xml_desc(1))
+                  updated_xml = String.new
+                  updated_xml_descr.write(updated_xml)
+
+                  updated = Nokogiri::XML(updated_xml, &:noblanks)
+
+                  pretty_proposed = StringIO.new
+                  pretty_updated = StringIO.new
+                  proposed.write_xml_to(pretty_proposed, indent: 2)
+                  updated.write_xml_to(pretty_updated, indent: 2)
+
+                  diff = Diffy::Diff.new(pretty_proposed.string, pretty_updated.string, :context => 3).to_s(:text)
+
+                  unless diff.empty?
+                    error_msg = "Libvirt failed to fully update the domain with the specified XML. Result differs from requested:\n" +
+                      "--- requested\n+++ result\n#{diff}\n" +
+                      "Typically this means there is a bug in the XML being sent, please log an issue"
+
+                    raise Errors::UpdateServerError, error_message: error_msg
+                  end
+                rescue Exception => e
+                  env[:machine].provider.driver.connection.define_domain(descr)
+                  raise
                 end
               end
             rescue Errors::VagrantLibvirtError => e
               env[:ui].error("Error when updating domain settings: #{e.message}")
+              raise
             end
             # Autostart with host if enabled in Vagrantfile
             libvirt_domain.autostart = config.autostart
             @logger.debug {
               "Starting Domain with XML:\n#{libvirt_domain.xml_desc}"
             }
             # Actually start the domain
+            env[:ui].info(I18n.t('vagrant_libvirt.starting_domain'))
             domain.start
           rescue Fog::Errors::Error, Errors::VagrantLibvirtError => e
             raise Errors::FogError, message: e.message

lib/vagrant-libvirt/errors.rb

@@ -109,7 +109,7 @@ class FogCreateDomainVolumeError < VagrantLibvirtError
       end
 
       class FogCreateServerError < VagrantLibvirtError
-        error_key(:fog_create_server_error)
+        error_key(:create_server_error)
       end
 
       # Network exceptions
@@ -154,6 +154,10 @@ class ManagementNetworkRequired < VagrantLibvirtError
       end
 
       # Other exceptions
+      class UpdateServerError < VagrantLibvirtError
+        error_key(:create_server_error)
+      end
+
       class InterfaceSlotNotAvailable < VagrantLibvirtError
         error_key(:interface_slot_not_available)
       end

locales/en.yml

@@ -25,6 +25,8 @@ en:
       Creating image (snapshot of base box volume).
     removing_domain_volume: |-
       Removing image (snapshot of base box volume).
+    updating_domain: |-
+      Updating domain definition due to configuration change
     starting_domain: |-
       Starting domain.
     terminating: |-
@@ -138,7 +140,7 @@ en:
         Error while creating a storage pool volume: %{error_message}
       fog_create_domain_volume_error: |-
         Error while creating volume for domain: %{error_message}
-      fog_create_server_error: |-
+      create_server_error: |-
         Error while creating domain: %{error_message}
       domain_name_exists: |-
         Name `%{domain_name}` of domain about to create is already taken. Please try to run

spec/unit/action/start_domain_spec.rb

@@ -33,8 +33,7 @@
       allow(servers).to receive(:get).and_return(domain)
 
       allow(logger).to receive(:debug)
-      expect(logger).to receive(:info)
-      expect(ui).to_not receive(:error)
+      allow(logger).to receive(:info)
 
       allow(libvirt_domain).to receive(:xml_desc).and_return(domain_xml)
 
@@ -43,23 +42,53 @@
     end
 
     it 'should execute without changing' do
-      expect(libvirt_domain).to_not receive(:undefine)
+      expect(ui).to_not receive(:error)
+      expect(libvirt_client).to_not receive(:define_domain_xml)
       expect(libvirt_domain).to receive(:autostart=)
       expect(domain).to receive(:start)
 
       expect(subject.call(env)).to be_nil
     end
 
-    context 'when previously running default config' do
-      let(:test_file) { 'existing.xml' }
+    context 'when any setting changed' do
+      let(:vagrantfile_providerconfig) do
+        <<-EOF
+        libvirt.cpus = 2
+        EOF
+      end
+
+      let(:updated_domain_xml) {
+        new_xml = domain_xml.dup
+        new_xml['<vcpu>1</vcpu>'] = '<vcpu>2</vcpu>'
+        new_xml
+      }
 
-      it 'should execute without changing' do
-        expect(libvirt_domain).to_not receive(:undefine)
+      it 'should update the domain' do
+        expect(ui).to_not receive(:error)
         expect(libvirt_domain).to receive(:autostart=)
+        expect(connection).to receive(:define_domain).and_return(libvirt_domain)
+        expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
         expect(domain).to receive(:start)
 
         expect(subject.call(env)).to be_nil
       end
+
+      context 'when there is an error during update' do
+        it 'should skip attempting to start' do
+          expect(ui).to receive(:error)
+          expect(connection).to receive(:define_domain).and_raise(::Libvirt::Error)
+
+          expect { subject.call(env) }.to raise_error(VagrantPlugins::ProviderLibvirt::Errors::VagrantLibvirtError)
+        end
+      end
+
+      context 'when there is an interrupt' do
+        it 'should skip attempting to start' do
+          expect(connection).to receive(:define_domain).and_raise(Interrupt)
+
+          expect { subject.call(env) }.to raise_error(Interrupt)
+        end
+      end
     end
 
     context 'nvram' do
@@ -73,9 +102,10 @@
         let(:test_file) { 'existing.xml' }
         let(:updated_test_file) { 'existing_added_nvram.xml' }
 
-        it 'should undefine without passing flags' do
-          expect(libvirt_domain).to receive(:undefine).with(0)
-          expect(servers).to receive(:create).with(xml: updated_domain_xml)
+        it 'should add the nvram element' do
+          expect(ui).to_not receive(:error)
+          expect(connection).to receive(:define_domain).with(updated_domain_xml).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -87,17 +117,16 @@
         let(:vagrantfile_providerconfig) do
           <<-EOF
           libvirt.loader = "/path/to/loader/file"
-          libvirt.nvram = "/path/to/nvram/file"
-          # change another setting to trigger the undefine/create
-          libvirt.cpus = 4
+          libvirt.nvram = "/path/to/nvram/file1"
           EOF
         end
         let(:test_file) { 'nvram_domain.xml' }
         let(:updated_test_file) { 'nvram_domain_other_setting.xml' }
 
-        it 'should set the flag to keep nvram' do
-          expect(libvirt_domain).to receive(:undefine).with(VagrantPlugins::ProviderLibvirt::Util::DomainFlags::VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)
-          expect(servers).to receive(:create).with(xml: updated_domain_xml)
+        it 'should keep the XML element' do
+          expect(ui).to_not receive(:error)
+          expect(connection).to receive(:define_domain).with(updated_domain_xml).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -108,9 +137,10 @@
           let(:vagrantfile_providerconfig) { }
           let(:updated_test_file) { 'nvram_domain_removed.xml' }
 
-          it 'should set the flag to remove nvram' do
-            expect(libvirt_domain).to receive(:undefine).with(VagrantPlugins::ProviderLibvirt::Util::DomainFlags::VIR_DOMAIN_UNDEFINE_NVRAM)
-            expect(servers).to receive(:create).with(xml: updated_domain_xml)
+          it 'should delete the XML element' do
+            expect(ui).to_not receive(:error)
+            expect(connection).to receive(:define_domain).with(updated_domain_xml).and_return(libvirt_domain)
+            expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
             expect(libvirt_domain).to receive(:autostart=)
             expect(domain).to receive(:start)
 
@@ -132,9 +162,10 @@
         end
 
         it 'should modify the domain tpm_path' do
-          expect(libvirt_domain).to receive(:undefine)
+          expect(ui).to_not receive(:error)
           expect(logger).to receive(:debug).with('tpm config changed')
-          expect(servers).to receive(:create).with(xml: updated_domain_xml)
+          expect(connection).to receive(:define_domain).with(updated_domain_xml).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -153,9 +184,10 @@
         end
 
         it 'should modify the domain tpm_path' do
-          expect(libvirt_domain).to receive(:undefine)
+          expect(ui).to_not receive(:error)
           expect(logger).to receive(:debug).with('tpm config changed')
-          expect(servers).to receive(:create).with(xml: updated_domain_xml)
+          expect(connection).to receive(:define_domain).with(updated_domain_xml).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -175,6 +207,7 @@
         end
 
         it 'should execute without changing' do
+          expect(ui).to_not receive(:error)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -194,6 +227,7 @@
         end
 
         it 'should execute without changing' do
+          expect(ui).to_not receive(:error)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -213,9 +247,10 @@
         end
 
         it 'should modify the domain' do
-          expect(libvirt_domain).to receive(:undefine)
+          expect(ui).to_not receive(:error)
           expect(logger).to receive(:debug).with('tpm config changed')
-          expect(servers).to receive(:create).with(xml: updated_domain_xml)
+          expect(connection).to receive(:define_domain).with(updated_domain_xml).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -235,8 +270,9 @@
         end
 
         it 'should not modify the domain' do
+          expect(ui).to_not receive(:error)
           expect(logger).to_not receive(:debug).with('clock timers config changed')
-          expect(servers).to_not receive(:create)
+          expect(connection).to_not receive(:define_domain)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -252,10 +288,13 @@
           EOF
         end
 
+        let(:updated_test_file) { 'clock_timer_rtc_tsc.xml' }
+
         it 'should modify the domain' do
-          expect(libvirt_domain).to receive(:undefine)
+          expect(ui).to_not receive(:error)
           expect(logger).to receive(:debug).with('clock timers config changed')
-          expect(servers).to receive(:create).with(xml: match(/<clock offset='utc'>\s*<timer name='rtc'\/>\s*<timer name='tsc'\/>\s*<\/clock>/))
+          expect(connection).to receive(:define_domain).with(match(/<clock offset='utc'>\s*<timer name='rtc'\/>\s*<timer name='tsc'\/>\s*<\/clock>/)).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -264,10 +303,13 @@
       end
 
       context 'timers removed' do
+        let(:updated_test_file) { 'clock_timer_removed.xml' }
+
         it 'should modify the domain' do
-          expect(libvirt_domain).to receive(:undefine)
+          expect(ui).to_not receive(:error)
           expect(logger).to receive(:debug).with('clock timers config changed')
-          expect(servers).to receive(:create).with(xml: match(/<clock offset='utc'>\s*<\/clock>/))
+          expect(connection).to receive(:define_domain).with(match(/<clock offset='utc'>\s*<\/clock>/)).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(domain_xml, updated_domain_xml)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)