Ensure shutdown timeout adjusted if graceful halt fails

lib/vagrant-libvirt/action.rb

@@ -139,18 +139,22 @@ module VagrantPlugins
               b3.use ResumeDomain if env2[:result]
             end
 
+            # only perform shutdown if VM is running
             b2.use Call, IsRunning do |env2, b3|
               next unless env2[:result]
 
-              # VM is running, halt it.
-              b3.use Call, GracefulHalt, :shutoff, :running do |env3, b4|
-                if !env3[:result]
-                  b4.use Call, ShutdownDomain, :shutoff, :running do |env4, b5|
-                    if !env4[:result]
-                      b5.use HaltDomain
-                    end
-                  end
-                end
+              b3.use Call, Message, "Attempting nice shutdowns..." do |_, b4|
+                # ShutdownDomain will perform the domain shutdown on the out calls
+                # so it runs after the remaining actions in the same action builder.
+                b4.use ShutdownDomain, :shutoff, :running
+                b4.use GracefulHalt, :shutoff, :running
+              end
+
+              # Only force halt if previous actions insufficient.
+              b3.use Call, IsRunning do |env3, b4|
+                next unless env3[:result]
+
+                b4.use HaltDomain
               end
             end
           end

lib/vagrant-libvirt/action/shutdown_domain.rb

@@ -14,6 +14,26 @@ module VagrantPlugins
 
         def call(env)
           timeout = env[:machine].config.vm.graceful_halt_timeout
+
+          start_time = Time.now
+
+          # call nested action first under the assumption it should try to
+          # handle shutdown via client capabilities
+          @app.call(env)
+
+          # return if successful, otherwise will ensure result is set to false
+          env[:result] = env[:machine].state.id == @target_state
+
+          return if env[:result]
+
+          current_time = Time.now
+
+          # if we've already exceeded the timeout
+          return if current_time - start_time >= timeout
+
+          # otherwise construct a new timeout.
+          timeout = timeout - (current_time - start_time)
+
           domain = env[:machine].provider.driver.connection.servers.get(env[:machine].id.to_s)
           if env[:machine].state.id == @source_state
             env[:ui].info(I18n.t('vagrant_libvirt.shutdown_domain'))
@@ -22,8 +42,6 @@ module VagrantPlugins
           end
 
           env[:result] = env[:machine].state.id == @target_state
-
-          @app.call(env)
         end
       end
     end

spec/unit/action/shutdown_domain_spec.rb

@@ -66,7 +66,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
 
       context "when final state is not shutoff" do
         before do
-          expect(domain).to receive(:state).and_return('running').exactly(4).times
+          expect(domain).to receive(:state).and_return('running').exactly(6).times
         end
 
         it "should provide a false result" do
@@ -77,8 +77,8 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
 
       context "when final state is shutoff" do
         before do
-          expect(domain).to receive(:state).and_return('running').exactly(3).times
-          expect(domain).to receive(:state).and_return('shutoff')
+          expect(domain).to receive(:state).and_return('running').exactly(4).times
+          expect(domain).to receive(:state).and_return('shutoff').exactly(2).times
         end
 
         it "should provide a true result" do
@@ -86,6 +86,38 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
           expect(env[:result]).to be_truthy
         end
       end
+
+      context "when timeout exceeded" 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(domain).to_not receive(:wait_for)
+          expect(domain).to_not receive(:shutdown)
+        end
+
+        it "should provide a false result" do
+          subject.call(env)
+          expect(env[:result]).to be_falsey
+        end
+      end
+
+      context "when timeout not exceeded" 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(domain).to receive(:wait_for) do |time|
+            expect(time).to be < 1
+            expect(time).to be > 0
+          end
+        end
+
+        it "should wait for the reduced time" do
+          subject.call(env)
+          expect(env[:result]).to be_falsey
+        end
+      end
     end
   end
 end

spec/unit/action_spec.rb

@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'support/sharedcontext'
+
+require 'vagrant/action/runner'
+
+require 'vagrant-libvirt/action'
+
+
+describe VagrantPlugins::ProviderLibvirt::Action do
+  subject { described_class }
+
+  include_context 'libvirt'
+  include_context 'unit'
+
+  let(:libvirt_domain) { double('libvirt_domain') }
+  let(:runner) { Vagrant::Action::Runner.new(env) }
+  let(:state) { double('state') }
+
+  before do
+    allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver)
+      .to receive(:connection).and_return(connection)
+    allow(machine).to receive(:id).and_return('test-machine-id')
+    allow(machine).to receive(:state).and_return(state)
+
+    allow(logger).to receive(:info)
+    allow(logger).to receive(:debug)
+    allow(logger).to receive(:error)
+  end
+
+  def allow_action_env_result(action, *responses)
+    results = responses.dup
+
+    allow_any_instance_of(action).to receive(:call) do |cls, env|
+      app = cls.instance_variable_get(:@app)
+
+      env[:result] = results[0]
+      if results.length > 1
+        results.shift
+      end
+
+      app.call(env)
+    end
+  end
+
+  describe '#action_halt' do
+    context 'not created' do
+      before do
+        expect(state).to receive(:id).and_return(:not_created)
+      end
+
+      it 'should execute without error' do
+        expect(ui).to receive(:info).with('Domain is not created. Please run `vagrant up` first.')
+
+        expect { runner.run(subject.action_halt) }.not_to raise_error
+      end
+    end
+
+    context 'running' do
+      before do
+        allow_action_env_result(VagrantPlugins::ProviderLibvirt::Action::IsCreated, true)
+        allow_action_env_result(VagrantPlugins::ProviderLibvirt::Action::IsSuspended, false)
+        allow_action_env_result(VagrantPlugins::ProviderLibvirt::Action::IsRunning, true)
+      end
+
+      context 'when shutdown domain works' do
+        before do
+          allow_action_env_result(VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain, true)
+          allow_action_env_result(Vagrant::Action::Builtin::GracefulHalt, true)
+          allow_action_env_result(VagrantPlugins::ProviderLibvirt::Action::IsRunning, true, false)
+        end
+
+        it 'should skip calling HaltDomain' do
+          expect(ui).to_not receive(:info).with('Domain is not created. Please run `vagrant up` first.')
+          expect_any_instance_of(VagrantPlugins::ProviderLibvirt::Action::HaltDomain).to_not receive(:call)
+
+          expect { runner.run(subject.action_halt) }.not_to raise_error
+        end
+      end
+
+      context 'when shutdown domain fails' do
+        before do
+          allow_action_env_result(VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain, false)
+          allow_action_env_result(Vagrant::Action::Builtin::GracefulHalt, false)
+        end
+
+        it 'should call halt' do
+          expect_any_instance_of(VagrantPlugins::ProviderLibvirt::Action::HaltDomain).to receive(:call)
+
+          expect { runner.run(subject.action_halt) }.not_to raise_error
+        end
+      end
+    end
+  end
+end