From c7bcb50b2b3ed20e39d5607597d44bd7cccfa55f Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Fri, 3 Jun 2022 10:42:28 +0100 Subject: [PATCH] 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 | 55 +++++++--- lib/vagrant-libvirt/errors.rb | 6 +- locales/en.yml | 4 +- spec/unit/action/start_domain_spec.rb | 102 ++++++++++++------ .../start_domain_spec/clock_timer_removed.xml | 38 +++++++ .../start_domain_spec/clock_timer_rtc_tsc.xml | 39 +++++++ .../nvram_domain_other_setting.xml | 4 +- vagrant-libvirt.gemspec | 1 + 8 files changed, 202 insertions(+), 47 deletions(-) create mode 100644 spec/unit/action/start_domain_spec/clock_timer_removed.xml create mode 100644 spec/unit/action/start_domain_spec/clock_timer_rtc_tsc.xml diff --git a/lib/vagrant-libvirt/action/start_domain.rb b/lib/vagrant-libvirt/action/start_domain.rb index b7458a7..dc89934 100644 --- a/lib/vagrant-libvirt/action/start_domain.rb +++ b/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 diff --git a/lib/vagrant-libvirt/errors.rb b/lib/vagrant-libvirt/errors.rb index f006193..caf2203 100644 --- a/lib/vagrant-libvirt/errors.rb +++ b/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 diff --git a/locales/en.yml b/locales/en.yml index af8d2a4..75346d5 100644 --- a/locales/en.yml +++ b/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 diff --git a/spec/unit/action/start_domain_spec.rb b/spec/unit/action/start_domain_spec.rb index 221e5b2..ac30a83 100644 --- a/spec/unit/action/start_domain_spec.rb +++ b/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['1'] = '2' + 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(/\s*\s*\s*<\/clock>/)) + expect(connection).to receive(:define_domain).with(match(/\s*\s*\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(/\s*<\/clock>/)) + expect(connection).to receive(:define_domain).with(match(/\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) diff --git a/spec/unit/action/start_domain_spec/clock_timer_removed.xml b/spec/unit/action/start_domain_spec/clock_timer_removed.xml new file mode 100644 index 0000000..051e61a --- /dev/null +++ b/spec/unit/action/start_domain_spec/clock_timer_removed.xml @@ -0,0 +1,38 @@ + + + + <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> diff --git a/spec/unit/action/start_domain_spec/clock_timer_rtc_tsc.xml b/spec/unit/action/start_domain_spec/clock_timer_rtc_tsc.xml new file mode 100644 index 0000000..206f6d0 --- /dev/null +++ b/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> diff --git a/spec/unit/action/start_domain_spec/nvram_domain_other_setting.xml b/spec/unit/action/start_domain_spec/nvram_domain_other_setting.xml index 9b80562..3aeccfd 100644 --- a/spec/unit/action/start_domain_spec/nvram_domain_other_setting.xml +++ b/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> diff --git a/vagrant-libvirt.gemspec b/vagrant-libvirt.gemspec index 07eac93..5e1abb5 100644 --- a/vagrant-libvirt.gemspec +++ b/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'