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'