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

@@ -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

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)

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).
+      before { expect(driver).to receive(:state).at_least(1).
-          and_return('not_created') }
+          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).
+        expect(driver).to receive(:state).and_return(:running)
-          and_return('running')
-        allow(domain).to receive(:poweroff)
       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

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(driver).to receive(:state).and_return(:running)
-        allow(domain).to receive(:wait_for)
-        allow(domain).to receive(:shutdown)
       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(driver).to receive(:state).and_return(:running).exactly(2).times
-          expect(domain).to receive(:state).and_return('shutoff').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

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