Handle different attribute and element ordering

Normalise the XML to ensure the attributes for both documents have the same ordering to prevent excessive noise when differences are detected. Additionally sort various elements based on attributes that make ordering irrelevant to allow for simpler comparison using xmlsimple.
vagrant-libvirt · Sep 9, 2022 · d544ed2 · d544ed2
1 parent 281422f
commit d544ed2
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 15 deletions.
diff --git a/lib/vagrant-libvirt/action/start_domain.rb b/lib/vagrant-libvirt/action/start_domain.rb
@@ -429,9 +429,20 @@ def call(env)
               raise Errors::UpdateServerError, error_message: e.message
             end
 
+            # this normalises the attribute order to be the same as what was sent in the above
+            # request to update the domain XML. Without this, if the XML documents are not
+            # equivalent, many more differences will be reported than there actually are.
+            applied_xml = String.new
+            REXML::Document.new(libvirt_domain.xml_desc(1)).write(applied_xml)
+
             # need to check whether the updated XML contains all the changes requested
             proposed = VagrantPlugins::ProviderLibvirt::Util::Xml.new(new_xml)
-            applied = VagrantPlugins::ProviderLibvirt::Util::Xml.new(libvirt_domain.xml_desc(1))
+            applied = VagrantPlugins::ProviderLibvirt::Util::Xml.new(applied_xml)
+
+            # perform some sorting to allow comparison otherwise order of devices differing
+            # even if they are equivalent will be reported as being different.
+            proposed.xml['devices'][0].each { |_, v| next unless v.is_a?(Array); v.sort_by! { |e| [e['type'], e['index']]} }
+            applied.xml['devices'][0].each { |_, v| next unless v.is_a?(Array); v.sort_by! { |e| [e['type'], e['index']]} }
 
             if proposed != applied
               require 'diffy'

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -61,6 +61,10 @@ def branch_coverage?
 
   # don't run acceptance tests by default
   config.filter_run_excluding :acceptance => true
+
+  config.expect_with :rspec do |c|
+    c.max_formatted_output_length = 2000
+  end
 end
 
 require 'vagrant-spec/unit'

diff --git a/spec/unit/action/start_domain_spec.rb b/spec/unit/action/start_domain_spec.rb
@@ -42,7 +42,7 @@
     end
 
     it 'should execute without changing' do
-      expect(ui).to_not receive(:error)
+      expect(ui).to_not receive(:warn)
       expect(libvirt_client).to_not receive(:define_domain_xml)
       expect(libvirt_domain).to receive(:autostart=)
       expect(domain).to receive(:start)
@@ -64,7 +64,27 @@
       end
 
       it 'should correctly detect the domain was updated' do
-        expect(ui).to_not receive(:error)
+        expect(ui).to_not receive(:warn)
+        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
+    end
+
+    context 'when xml elements and attributes reordered' do
+      let(:test_file) { 'existing.xml' }
+      let(:updated_test_file) { 'existing_reordered.xml' }
+      let(:vagrantfile_providerconfig) do
+        <<-EOF
+        libvirt.cpu_mode = "host-passthrough"
+        EOF
+      end
+
+      it 'should correctly detect the domain was updated' do
+        expect(ui).to_not receive(:warn)
         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)
@@ -115,7 +135,7 @@
       }
 
       it 'should update the domain' do
-        expect(ui).to_not receive(:error)
+        expect(ui).to_not receive(:warn)
         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)
@@ -154,7 +174,7 @@
         let(:updated_test_file) { 'existing_added_nvram.xml' }
 
         it 'should add the nvram element' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           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=)
@@ -175,7 +195,7 @@
         let(:updated_test_file) { 'nvram_domain_other_setting.xml' }
 
         it 'should keep the XML element' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           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=)
@@ -189,7 +209,7 @@
           let(:updated_test_file) { 'nvram_domain_removed.xml' }
 
           it 'should delete the XML element' do
-            expect(ui).to_not receive(:error)
+            expect(ui).to_not receive(:warn)
             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=)
@@ -213,7 +233,7 @@
         end
 
         it 'should modify the domain tpm_path' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(logger).to receive(:debug).with('tpm config changed')
           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)
@@ -235,7 +255,7 @@
         end
 
         it 'should modify the domain tpm_path' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(logger).to receive(:debug).with('tpm config changed')
           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)
@@ -258,7 +278,7 @@
         end
 
         it 'should execute without changing' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -278,7 +298,7 @@
         end
 
         it 'should execute without changing' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(libvirt_domain).to receive(:autostart=)
           expect(domain).to receive(:start)
 
@@ -298,7 +318,7 @@
         end
 
         it 'should modify the domain' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(logger).to receive(:debug).with('tpm config changed')
           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)
@@ -321,7 +341,7 @@
         end
 
         it 'should not modify the domain' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(logger).to_not receive(:debug).with('clock timers config changed')
           expect(connection).to_not receive(:define_domain)
           expect(libvirt_domain).to receive(:autostart=)
@@ -342,7 +362,7 @@
         let(:updated_test_file) { 'clock_timer_rtc_tsc.xml' }
 
         it 'should modify the domain' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(logger).to receive(:debug).with('clock timers config changed')
           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)
@@ -357,7 +377,7 @@
         let(:updated_test_file) { 'clock_timer_removed.xml' }
 
         it 'should modify the domain' do
-          expect(ui).to_not receive(:error)
+          expect(ui).to_not receive(:warn)
           expect(logger).to receive(:debug).with('clock timers config changed')
           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)

diff --git a/spec/unit/action/start_domain_spec/existing_reordered.xml b/spec/unit/action/start_domain_spec/existing_reordered.xml
@@ -0,0 +1,62 @@
+<domain type='qemu'>
+  <name>vagrant-libvirt_default</name>
+  <uuid>881a931b-0110-4d10-81aa-47a1a19f5726</uuid>
+  <description>Source: /home/test/vagrant-libvirt/Vagrantfile</description>
+  <memory unit='KiB'>2097152</memory>
+  <currentMemory unit='KiB'>2097152</currentMemory>
+  <vcpu placement='static'>2</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-i440fx-6.0'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <cpu check='partial' mode='host-passthrough'/>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <disk device='disk' type='file'>
+      <driver name='qemu' type='qcow2'/>
+      <source file='/var/lib/libvirt/images/vagrant-libvirt_default.img'/>
+      <target bus='virtio' dev='vda'/>
+      <address bus='0x00' domain='0x0000' function='0x0' slot='0x03' type='pci'/>
+    </disk>
+    <controller index='0' model='pci-root' type='pci'/>
+    <controller index='0' model='piix3-uhci' type='usb'>
+      <address bus='0x00' domain='0x0000' function='0x2' slot='0x01' type='pci'/>
+    </controller>
+    <interface type='network'>
+      <mac address='52:54:00:7d:14:0e'/>
+      <source network='vagrant-libvirt'/>
+      <model type='virtio'/>
+      <address bus='0x00' domain='0x0000' function='0x0' slot='0x05' type='pci'/>
+    </interface>
+    <serial type='pty'>
+      <target port='0' type='isa-serial'>
+        <model name='isa-serial'/>
+      </target>
+    </serial>
+    <console type='pty'>
+      <target port='0' type='serial'/>
+    </console>
+    <input bus='ps2' type='mouse'/>
+    <input bus='ps2' type='keyboard'/>
+    <graphics autoport='yes' keymap='en-us' listen='127.0.0.1' port='-1' type='vnc'>
+      <listen address='127.0.0.1' type='address'/>
+    </graphics>
+    <audio id='1' type='none'/>
+    <video>
+      <model heads='1' primary='yes' type='cirrus' vram='16384'/>
+      <address bus='0x00' domain='0x0000' function='0x0' slot='0x02' type='pci'/>
+    </video>
+    <memballoon model='virtio'>
+      <address bus='0x00' domain='0x0000' function='0x0' slot='0x04' type='pci'/>
+    </memballoon>
+  </devices>
+</domain>