From 606b86df67e2a3b452796a38628cb17045a950b6 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Sat, 3 Sep 2022 16:11:11 +0100 Subject: [PATCH] Return early from set_boot_order if not needed (#1578) Switch to calling the returning the next middleware in the chain as soon as possible in the set boot order action. Makes the overall remaining logic tidier. Include basic tests to check existing behaviour. --- lib/vagrant-libvirt/action/set_boot_order.rb | 86 ++++++++++--------- spec/unit/action/set_boot_order_spec.rb | 67 +++++++++++++++ .../action/set_boot_order_spec/default.xml | 76 ++++++++++++++++ .../explicit_boot_order.xml | 77 +++++++++++++++++ 4 files changed, 264 insertions(+), 42 deletions(-) create mode 100644 spec/unit/action/set_boot_order_spec.rb create mode 100644 spec/unit/action/set_boot_order_spec/default.xml create mode 100644 spec/unit/action/set_boot_order_spec/explicit_boot_order.xml diff --git a/lib/vagrant-libvirt/action/set_boot_order.rb b/lib/vagrant-libvirt/action/set_boot_order.rb index 4d89a1a..ea4faa9 100644 --- a/lib/vagrant-libvirt/action/set_boot_order.rb +++ b/lib/vagrant-libvirt/action/set_boot_order.rb @@ -16,6 +16,12 @@ module VagrantPlugins end def call(env) + # Only execute specific boot ordering if this is defined + # in the Vagrant file + unless @boot_order.count >= 1 + return @app.call(env) + end + # Get domain first begin domain = env[:machine].provider @@ -30,53 +36,49 @@ module VagrantPlugins error_message: e.message end - # Only execute specific boot ordering if this is defined - # in the Vagrant file - if @boot_order.count >= 1 + # If a domain is initially defined with no box or disk or + # with an explicit boot order, Libvirt adds + # This conflicts with an explicit boot_order configuration, + # so we need to remove it from the domain xml and feed it back. + # Also see https://bugzilla.redhat.com/show_bug.cgi?id=1248514 + # as to why we have to do this after all devices have been defined. + xml = Nokogiri::XML(domain.xml_desc) + xml.search('/domain/os/boot').each(&:remove) - # If a domain is initially defined with no box or disk or - # with an explicit boot order, Libvirt adds - # This conflicts with an explicit boot_order configuration, - # so we need to remove it from the domain xml and feed it back. - # Also see https://bugzilla.redhat.com/show_bug.cgi?id=1248514 - # as to why we have to do this after all devices have been defined. - xml = Nokogiri::XML(domain.xml_desc) - xml.search('/domain/os/boot').each(&:remove) + # Parse the XML and find each defined drive and network interfacee + hd = xml.search("/domain/devices/disk[@device='disk']") + cdrom = xml.search("/domain/devices/disk[@device='cdrom']") + # implemented only for 1 network + nets = @boot_order.flat_map do |x| + x.class == Hash ? x : nil + end.compact + raise 'Defined only for 1 network for boot' if nets.size > 1 + network = search_network(nets, xml) - # Parse the XML and find each defined drive and network interfacee - hd = xml.search("/domain/devices/disk[@device='disk']") - cdrom = xml.search("/domain/devices/disk[@device='cdrom']") - # implemented only for 1 network - nets = @boot_order.flat_map do |x| - x.class == Hash ? x : nil - end.compact - raise 'Defined only for 1 network for boot' if nets.size > 1 - network = search_network(nets, xml) + # Generate an array per device group and a flattened + # array from all of those + devices = { 'hd' => hd, + 'cdrom' => cdrom, + 'network' => network } - # Generate an array per device group and a flattened - # array from all of those - devices = { 'hd' => hd, - 'cdrom' => cdrom, - 'network' => network } - - final_boot_order = final_boot_order(@boot_order, devices) - # Loop over the entire defined boot order array and - # create boot order entries in the domain XML - final_boot_order.each_with_index do |node, index| - boot = "" - node.add_child(boot) - logger_msg(node, index) - end - - # Finally redefine the domain XML through Libvirt - # to apply the boot ordering - env[:machine].provider - .driver - .connection - .client - .define_domain_xml(xml.to_s) + final_boot_order = final_boot_order(@boot_order, devices) + # Loop over the entire defined boot order array and + # create boot order entries in the domain XML + final_boot_order.each_with_index do |node, index| + next if node.nil? + boot = "" + node.add_child(boot) + logger_msg(node, index) end + # Finally redefine the domain XML through Libvirt + # to apply the boot ordering + env[:machine].provider + .driver + .connection + .client + .define_domain_xml(xml.to_s) + @app.call(env) end diff --git a/spec/unit/action/set_boot_order_spec.rb b/spec/unit/action/set_boot_order_spec.rb new file mode 100644 index 0000000..7bd7d97 --- /dev/null +++ b/spec/unit/action/set_boot_order_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'support/sharedcontext' +require 'support/libvirt_context' + +require 'vagrant-libvirt/action/set_boot_order' + +describe VagrantPlugins::ProviderLibvirt::Action::SetBootOrder do + subject { described_class.new(app, env) } + + include_context 'unit' + include_context 'libvirt' + + #before do + # allow(driver).to receive(:created?).and_return(true) + #end + + describe '#call' do + it 'should return early' do + expect(connection).to_not receive(:client) + + expect(subject.call(env)).to be_nil + end + + context 'with boot_order defined' do + let(:domain_xml) { File.read(File.join(File.dirname(__FILE__), File.basename(__FILE__, '.rb'), test_file)) } + let(:updated_domain_xml) { File.read(File.join(File.dirname(__FILE__), File.basename(__FILE__, '.rb'), updated_test_file)) } + let(:test_file) { 'default.xml' } + let(:updated_test_file) { 'explicit_boot_order.xml' } + let(:vagrantfile_providerconfig) do + <<-EOF + libvirt.boot "hd" + libvirt.boot "cdrom" + libvirt.boot "network" => 'vagrant-libvirt' + EOF + end + + before do + allow(connection).to receive(:client).and_return(libvirt_client) + allow(libvirt_client).to receive(:lookup_domain_by_uuid).and_return(libvirt_domain) + allow(libvirt_domain).to receive(:xml_desc).and_return(domain_xml) + allow(logger).to receive(:debug) + end + + it 'should configure the boot order' do + expect(libvirt_client).to receive(:define_domain_xml).with(updated_domain_xml) + expect(subject.call(env)).to be_nil + end + + context 'with multiple networks in bootorder' do + let(:vagrantfile_providerconfig) do + <<-EOF + libvirt.boot "hd" + libvirt.boot "cdrom" + libvirt.boot "network" => 'vagrant-libvirt' + libvirt.boot "network" => 'vagrant-libvirt' + EOF + end + + it 'should raise an exception' do + expect { subject.call(env) }.to raise_error('Defined only for 1 network for boot') + end + end + end + end +end diff --git a/spec/unit/action/set_boot_order_spec/default.xml b/spec/unit/action/set_boot_order_spec/default.xml new file mode 100644 index 0000000..fbeb9ed --- /dev/null +++ b/spec/unit/action/set_boot_order_spec/default.xml @@ -0,0 +1,76 @@ + + vagrant-libvirt_default + 881a931b-0110-4d10-81aa-47a1a19f5726 + Source: /home/test/vagrant-libvirt/Vagrantfile + 2097152 + 2097152 + 2 + + hvm + + + + + + + + + + destroy + restart + destroy + + /usr/bin/qemu-system-x86_64 + + + + +
+ + + + + + +
+ + + + + + +
+ + +
+ + + + + + +
+ + + + + + + + + + + + + + +