From 81b6fb715a661ecbd85f1b94177029ca52315b52 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Thu, 30 Sep 2021 13:35:30 +0100 Subject: [PATCH] Handle VM not accessible during reboot (#1367) To support commands requesting a reboot of a VM after execution, the query of ssh_info needs to avoid triggering an error when the IP address is not yet retrievable as this indicates the VM would not be reachable. Wrap the returning of the state in the driver to distinguish between the following states: - :running - indicates the machine is available - :inaccessible - the machine is running but not yet available to connect This is based on the behaviour from the virtualbox provider. Includes some rudimentary tests to exercise the driver state code. Closes: #1366 --- lib/vagrant-libvirt/driver.rb | 12 +++- spec/unit/action/destroy_domain_spec.rb | 8 ++- spec/unit/action/halt_domain_spec.rb | 18 +++--- spec/unit/action/shutdown_domain_spec.rb | 40 +++++++------ spec/unit/driver_spec.rb | 72 ++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 26 deletions(-) diff --git a/lib/vagrant-libvirt/driver.rb b/lib/vagrant-libvirt/driver.rb index 5d800cb..672aab6 100644 --- a/lib/vagrant-libvirt/driver.rb +++ b/lib/vagrant-libvirt/driver.rb @@ -137,7 +137,17 @@ module VagrantPlugins # TODO: terminated no longer appears to be a valid fog state, remove? return :not_created if domain.nil? || domain.state.to_sym == :terminated - domain.state.tr('-', '_').to_sym + state = domain.state.tr('-', '_').to_sym + if state == :running + begin + get_domain_ipaddress(machine, domain) + rescue Fog::Errors::TimeoutError => e + @logger.debug("Machine #{machine.id} running but no IP address available: #{e}.") + return :inaccessible + end + end + + return state end private diff --git a/spec/unit/action/destroy_domain_spec.rb b/spec/unit/action/destroy_domain_spec.rb index 755cfc4..1d5873f 100644 --- a/spec/unit/action/destroy_domain_spec.rb +++ b/spec/unit/action/destroy_domain_spec.rb @@ -12,14 +12,18 @@ describe VagrantPlugins::ProviderLibvirt::Action::DestroyDomain do include_context 'unit' include_context 'libvirt' + let(:driver) { double('driver') } let(:libvirt_domain) { double('libvirt_domain') } let(:libvirt_client) { double('libvirt_client') } let(:servers) { double('servers') } + before do + allow(machine.provider).to receive('driver').and_return(driver) + allow(driver).to receive(:connection).and_return(connection) + end + describe '#call' do before do - allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver) - .to receive(:connection).and_return(connection) allow(connection).to receive(:client).and_return(libvirt_client) allow(libvirt_client).to receive(:lookup_domain_by_uuid) .and_return(libvirt_domain) diff --git a/spec/unit/action/halt_domain_spec.rb b/spec/unit/action/halt_domain_spec.rb index 24f9866..5f6f673 100644 --- a/spec/unit/action/halt_domain_spec.rb +++ b/spec/unit/action/halt_domain_spec.rb @@ -11,21 +11,26 @@ describe VagrantPlugins::ProviderLibvirt::Action::HaltDomain do include_context 'unit' include_context 'libvirt' + let(:driver) { double('driver') } let(:libvirt_domain) { double('libvirt_domain') } let(:servers) { double('servers') } + before do + allow(machine.provider).to receive('driver').and_return(driver) + allow(driver).to receive(:created?).and_return(true) + allow(driver).to receive(:connection).and_return(connection) + end + describe '#call' do before do - allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver) - .to receive(:connection).and_return(connection) allow(connection).to receive(:servers).and_return(servers) allow(servers).to receive(:get).and_return(domain) allow(ui).to receive(:info).with('Halting domain...') end context "when state is not running" do - before { expect(domain).to receive(:state).at_least(1). - and_return('not_created') } + before { expect(driver).to receive(:state).at_least(1). + and_return(:not_created) } it "should not poweroff when state is not running" do expect(domain).not_to receive(:poweroff) @@ -40,9 +45,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::HaltDomain do context "when state is running" do before do - expect(domain).to receive(:state).at_least(1). - and_return('running') - allow(domain).to receive(:poweroff) + expect(driver).to receive(:state).and_return(:running) end it "should poweroff" do @@ -51,6 +54,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::HaltDomain do end it "should print halting message" do + allow(domain).to receive(:poweroff) expect(ui).to receive(:info).with('Halting domain...') subject.call(env) end diff --git a/spec/unit/action/shutdown_domain_spec.rb b/spec/unit/action/shutdown_domain_spec.rb index 32bb7d7..7997455 100644 --- a/spec/unit/action/shutdown_domain_spec.rb +++ b/spec/unit/action/shutdown_domain_spec.rb @@ -9,22 +9,29 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do include_context 'unit' include_context 'libvirt' + let(:driver) { double('driver') } let(:libvirt_domain) { double('libvirt_domain') } let(:servers) { double('servers') } let(:current_state) { :running } let(:target_state) { :shutoff } + before do + allow(machine.provider).to receive('driver').and_return(driver) + allow(driver).to receive(:created?).and_return(true) + allow(driver).to receive(:connection).and_return(connection) + end + describe '#call' do before do - allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver) - .to receive(:connection).and_return(connection) allow(connection).to receive(:servers).and_return(servers) allow(servers).to receive(:get).and_return(domain) allow(ui).to receive(:info).with('Attempting direct shutdown of domain...') end context "when state is shutoff" do - before { allow(domain).to receive(:state).and_return('shutoff') } + before do + allow(driver).to receive(:state).and_return(:shutoff) + end it "should not shutdown" do expect(domain).not_to receive(:shutoff) @@ -44,29 +51,27 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do context "when state is running" do before do - allow(domain).to receive(:state).and_return('running') - allow(domain).to receive(:wait_for) - allow(domain).to receive(:shutdown) + allow(driver).to receive(:state).and_return(:running) end it "should shutdown" do + expect(domain).to receive(:wait_for) expect(domain).to receive(:shutdown) subject.call(env) end it "should print shutdown message" do - expect(ui).to receive(:info).with('Attempting direct shutdown of domain...') - subject.call(env) - end - - it "should wait for machine to shutdown" do expect(domain).to receive(:wait_for) + expect(domain).to receive(:shutdown) + expect(ui).to receive(:info).with('Attempting direct shutdown of domain...') subject.call(env) end context "when final state is not shutoff" do before do - expect(domain).to receive(:state).and_return('running').exactly(6).times + expect(driver).to receive(:state).and_return(:running).exactly(3).times + expect(domain).to receive(:wait_for) + expect(domain).to receive(:shutdown) end it "should provide a false result" do @@ -77,8 +82,10 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do context "when final state is shutoff" do before do - expect(domain).to receive(:state).and_return('running').exactly(4).times - expect(domain).to receive(:state).and_return('shutoff').exactly(2).times + expect(driver).to receive(:state).and_return(:running).exactly(2).times + expect(driver).to receive(:state).and_return(:shutoff).exactly(1).times + expect(domain).to receive(:wait_for) + expect(domain).to receive(:shutdown) end it "should provide a true result" do @@ -91,7 +98,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do before do expect(machine).to receive_message_chain('config.vm.graceful_halt_timeout').and_return(1) expect(app).to receive(:call) { sleep 1.5 } - expect(domain).to receive(:state).and_return('running').exactly(2).times + expect(driver).to receive(:state).and_return(:running).exactly(1).times expect(domain).to_not receive(:wait_for) expect(domain).to_not receive(:shutdown) end @@ -106,11 +113,12 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do before do expect(machine).to receive_message_chain('config.vm.graceful_halt_timeout').and_return(2) expect(app).to receive(:call) { sleep 1 } - expect(domain).to receive(:state).and_return('running').exactly(6).times + expect(driver).to receive(:state).and_return(:running).exactly(3).times expect(domain).to receive(:wait_for) do |time| expect(time).to be < 1 expect(time).to be > 0 end + expect(domain).to receive(:shutdown) end it "should wait for the reduced time" do diff --git a/spec/unit/driver_spec.rb b/spec/unit/driver_spec.rb index 4215d21..ace277b 100644 --- a/spec/unit/driver_spec.rb +++ b/spec/unit/driver_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'spec_helper' +require 'support/binding_proc' require 'support/sharedcontext' require 'vagrant-libvirt/driver' @@ -8,6 +9,8 @@ require 'vagrant-libvirt/driver' describe VagrantPlugins::ProviderLibvirt::Driver do include_context 'unit' + subject { described_class.new(machine) } + let(:vagrantfile) do <<-EOF Vagrant.configure('2') do |config| @@ -80,4 +83,73 @@ describe VagrantPlugins::ProviderLibvirt::Driver do expect(machine.provider.driver.system_connection).to eq(system_connection1) end end + + describe '#state' do + let(:domain) { double('domain') } + + before do + allow(subject).to receive(:get_domain).and_return(domain) + end + + [ + [ + 'not found', + :not_created, + { + :setup => ProcWithBinding.new do + expect(subject).to receive(:get_domain).and_return(nil) + end, + } + ], + [ + 'libvirt error', + :not_created, + { + :setup => ProcWithBinding.new do + expect(subject).to receive(:get_domain).and_raise(Libvirt::RetrieveError, 'missing') + end, + } + ], + [ + 'terminated', + :not_created, + { + :setup => ProcWithBinding.new do + expect(domain).to receive(:state).and_return('terminated') + end, + } + ], + [ + 'no IP returned', + :inaccessible, + { + :setup => ProcWithBinding.new do + expect(domain).to receive(:state).and_return('running').twice() + expect(subject).to receive(:get_domain_ipaddress).and_raise(Fog::Errors::TimeoutError) + end, + } + ], + [ + 'running', + :running, + { + :setup => ProcWithBinding.new do + expect(domain).to receive(:state).and_return('running').twice() + expect(subject).to receive(:get_domain_ipaddress).and_return('192.168.121.2') + end, + } + ], + ].each do |name, expected, options| + opts = {} + opts.merge!(options) if options + + it "should handle '#{name}' by returning '#{expected}'" do + if !opts[:setup].nil? + opts[:setup].apply_binding(binding) + end + + expect(subject.state(machine)).to eq(expected) + end + end + end end