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 @@ module VagrantPlugins
         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 @@ module VagrantPlugins
                 libvirt_domain.memory = libvirt_domain.max_memory
               end
             end
+
             begin
               # XML definition manipulation
               descr = libvirt_domain.xml_desc(1)
@@ -248,7 +248,7 @@ module VagrantPlugins
               # 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 @@ module VagrantPlugins
                 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,28 +406,57 @@ module VagrantPlugins
                     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
-                  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
+                  # 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
+                  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
@@ -436,6 +464,7 @@ module VagrantPlugins
               "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 @@ module VagrantPlugins
       end
 
       class FogCreateServerError < VagrantLibvirtError
-        error_key(:fog_create_server_error)
+        error_key(:create_server_error)
       end
 
       # Network exceptions
@@ -154,6 +154,10 @@ module VagrantPlugins
       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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
       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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
     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
 
-      it 'should execute without changing' do
-        expect(libvirt_domain).to_not receive(:undefine)
+      let(:updated_domain_xml) {
+        new_xml = domain_xml.dup
+        new_xml['<vcpu>1</vcpu>'] = '<vcpu>2</vcpu>'
+        new_xml
+      }
+
+      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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
           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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
         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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
           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 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
       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)
 

spec/unit/action/start_domain_spec/clock_timer_removed.xml

@@ -0,0 +1,38 @@
+<domain xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0' type=''>
+  <name/>
+  <title/>
+  <description/>
+  <uuid/>
+  <memory/>
+  <vcpu>1</vcpu>
+  <cpu mode='host-model'>
+    <model fallback='allow'/>
+  </cpu>
+  <os>
+    <type>hvm</type>
+    <kernel/>
+    <initrd/>
+    <cmdline/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset='utc'>
+    
+  </clock>
+  <devices>
+    <serial type='pty'>
+      <target port='0'/>
+    </serial>
+    <console type='pty'>
+      <target port='0'/>
+    </console>
+    <input bus='ps2' type='mouse'/>
+    <graphics autoport='yes' keymap='en-us' listen='127.0.0.1' port='-1' type='vnc'/>
+    <video>
+      <model heads='1' type='cirrus' vram='16384'/>
+    </video>
+  </devices>
+</domain>

spec/unit/action/start_domain_spec/clock_timer_rtc_tsc.xml

@@ -0,0 +1,39 @@
+<domain xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0' type=''>
+  <name/>
+  <title/>
+  <description/>
+  <uuid/>
+  <memory/>
+  <vcpu>1</vcpu>
+  <cpu mode='host-model'>
+    <model fallback='allow'/>
+  </cpu>
+  <os>
+    <type>hvm</type>
+    <kernel/>
+    <initrd/>
+    <cmdline/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset='utc'>
+    <timer name='rtc'/>
+    <timer name='tsc'/>
+  </clock>
+  <devices>
+    <serial type='pty'>
+      <target port='0'/>
+    </serial>
+    <console type='pty'>
+      <target port='0'/>
+    </console>
+    <input bus='ps2' type='mouse'/>
+    <graphics autoport='yes' keymap='en-us' listen='127.0.0.1' port='-1' type='vnc'/>
+    <video>
+      <model heads='1' type='cirrus' vram='16384'/>
+    </video>
+  </devices>
+</domain>

spec/unit/action/start_domain_spec/nvram_domain_other_setting.xml

@@ -4,11 +4,11 @@
   <description>Source: /home/test/vagrant-libvirt/Vagrantfile</description>
   <memory unit='KiB'>2097152</memory>
   <currentMemory unit='KiB'>2097152</currentMemory>
-  <vcpu placement='static'>4</vcpu>
+  <vcpu placement='static'>2</vcpu>
   <os>
     <type arch='x86_64' machine='pc-i440fx-6.0'>hvm</type>
     <loader type='pflash'>/path/to/loader/file</loader>
-    <nvram>/path/to/nvram/file</nvram>
+    <nvram>/path/to/nvram/file1</nvram>
     <boot dev='hd'/>
   </os>
   <features>

vagrant-libvirt.gemspec

@@ -27,6 +27,7 @@ Gem::Specification.new do |s|
   s.add_runtime_dependency 'fog-libvirt', '>= 0.6.0'
   s.add_runtime_dependency 'fog-core', '~> 2'
   s.add_runtime_dependency 'rexml'
+  s.add_runtime_dependency 'diffy'
 
   # Make sure to allow use of the same version as Vagrant by being less specific
   s.add_runtime_dependency 'nokogiri', '~> 1.6'