Handle VM not accessible during reboot

lib/vagrant-libvirt/driver.rb

@@ -137,7 +137,17 @@ def state(machine)
         # 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 @@
   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 @@
   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 @@
 
     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 @@
       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 @@
   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 @@
 
     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 @@
 
       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 @@
         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 @@
         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,13 +1,16 @@
 # frozen_string_literal: true
 
 require 'spec_helper'
+require 'support/binding_proc'
 require 'support/sharedcontext'
 
 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 @@
       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