Merge pull request #1310 from chrisroberts/builtin-graceful

Use GracefulHalt builtin action

lib/vagrant-libvirt/action.rb

@@ -139,11 +139,23 @@ 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, Message, "Attempting nice shutdowns..." do |_, b4|
-              b3.use HaltDomain
+                # 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
         end
@@ -348,6 +360,7 @@ module VagrantPlugins
       autoload :ForwardPorts, action_root.join('forward_ports')
       autoload :ClearForwardedPorts, action_root.join('forward_ports')
       autoload :HaltDomain, action_root.join('halt_domain')
+      autoload :ShutdownDomain, action_root.join('shutdown_domain')
       autoload :HandleBoxImage, action_root.join('handle_box_image')
       autoload :HandleStoragePool, action_root.join('handle_storage_pool')
       autoload :RemoveLibvirtImage, action_root.join('remove_libvirt_image')

lib/vagrant-libvirt/action/halt_domain.rb

@@ -13,41 +13,9 @@ module VagrantPlugins
         end
 
         def call(env)
-          env[:ui].info(I18n.t('vagrant_libvirt.halt_domain'))
-
-          timeout = env[:machine].config.vm.graceful_halt_timeout
           domain = env[:machine].provider.driver.connection.servers.get(env[:machine].id.to_s)
-          raise Errors::NoDomainError if domain.nil?
+          if env[:machine].state.id == :running
-
+            env[:ui].info(I18n.t('vagrant_libvirt.halt_domain'))
-          if env[:force_halt]
-            domain.poweroff
-            return @app.call(env)
-          end
-
-          begin
-            Timeout.timeout(timeout) do
-              begin
-                env[:machine].guest.capability(:halt)
-              rescue Timeout::Error
-                raise
-              rescue
-                @logger.info('Trying Libvirt graceful shutdown.')
-                # Read domain object again
-                dom = env[:machine].provider.driver.connection.servers.get(env[:machine].id.to_s)
-                if dom.state.to_s == 'running'
-                  dom.shutdown
-                end
-              end
-
-              domain.wait_for(timeout) do
-                !ready?
-              end
-            end
-          rescue Timeout::Error
-            @logger.info('VM is still running. Calling force poweroff.')
-            domain.poweroff
-          rescue
-            @logger.error('Failed to shutdown cleanly. Calling force poweroff.')
             domain.poweroff
           end
 

lib/vagrant-libvirt/action/shutdown_domain.rb

@@ -0,0 +1,49 @@
+require 'log4r'
+
+module VagrantPlugins
+  module ProviderLibvirt
+    module Action
+      # Shutdown the domain.
+      class ShutdownDomain
+        def initialize(app, _env, target_state, source_state)
+          @logger = Log4r::Logger.new('vagrant_libvirt::action::shutdown_domain')
+          @target_state = target_state
+          @source_state = source_state
+          @app = app
+        end
+
+        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'))
+            domain.shutdown
+            domain.wait_for(timeout) { !ready? }
+          end
+
+          env[:result] = env[:machine].state.id == @target_state
+        end
+      end
+    end
+  end
+end

locales/en.yml

@@ -29,6 +29,8 @@ en:
       Poweroff domain.
     destroy_domain: |-
       Removing domain...
+    shutdown_domain: |-
+      Attempting direct shutdown of domain...
     halt_domain: |-
       Halting domain...
     resuming_domain: |-

spec/unit/action/halt_domain_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require 'support/sharedcontext'
 require 'support/libvirt_context'
-require 'vagrant-libvirt/action/destroy_domain'
+require 'vagrant-libvirt/action/halt_domain'
 
 describe VagrantPlugins::ProviderLibvirt::Action::HaltDomain do
   subject { described_class.new(app, env) }
@@ -20,72 +20,39 @@ describe VagrantPlugins::ProviderLibvirt::Action::HaltDomain do
         .to receive(:connection).and_return(connection)
       allow(connection).to receive(:servers).and_return(servers)
       allow(servers).to receive(:get).and_return(domain)
-      # always see this at the start of #call
+      allow(ui).to receive(:info).with('Halting domain...')
-      expect(ui).to receive(:info).with('Halting domain...')
     end
 
-    context 'with graceful timeout' do
+    context "when state is not running" do
-      it "should shutdown" do
+      before { expect(domain).to receive(:state).at_least(1).
-        expect(guest).to receive(:capability).with(:halt).and_return(true)
+          and_return('not_created') }
-        expect(domain).to receive(:wait_for).with(60).and_return(false)
+
-        expect(subject.call(env)).to be_nil
+      it "should not poweroff when state is not running" do
+        expect(domain).not_to receive(:poweroff)
+        subject.call(env)
       end
 
-      context 'when halt fails' do
+      it "should not print halting message" do
-        before do
+        expect(ui).not_to receive(:info)
-          expect(logger).to receive(:info).with('Trying Libvirt graceful shutdown.')
+        subject.call(env)
-          expect(guest).to receive(:capability).with(:halt).and_raise(IOError)
-          expect(domain).to receive(:state).and_return('running')
-        end
-
-        it "should call shutdown" do
-          expect(domain).to receive(:shutdown)
-          expect(domain).to receive(:wait_for).with(60).and_return(false)
-          expect(subject.call(env)).to be_nil
-        end
-
-        context 'when shutdown fails' do
-          it "should call power off" do
-            expect(logger).to receive(:error).with('Failed to shutdown cleanly. Calling force poweroff.')
-            expect(domain).to receive(:shutdown).and_raise(IOError)
-            expect(domain).to receive(:poweroff)
-            expect(subject.call(env)).to be_nil
-          end
-        end
-
-        context 'when shutdown exceeds the timeout' do
-          it "should call poweroff" do
-            expect(logger).to receive(:info).with('VM is still running. Calling force poweroff.')
-            expect(domain).to receive(:shutdown).and_raise(Timeout::Error)
-            expect(domain).to receive(:poweroff)
-            expect(subject.call(env)).to be_nil
-          end
-        end
-      end
-
-      context 'when halt exceeds the timeout' do
-        before do
-          expect(logger).to_not receive(:info).with('Trying Libvirt graceful shutdown.')
-          expect(guest).to receive(:capability).with(:halt).and_raise(Timeout::Error)
-        end
-
-        it "should call poweroff" do
-          expect(logger).to receive(:info).with('VM is still running. Calling force poweroff.')
-          expect(domain).to receive(:poweroff)
-          expect(subject.call(env)).to be_nil
-        end
       end
     end
 
-    context 'with force halt enabled' do
+    context "when state is running" do
       before do
-        allow(env).to receive(:[]).and_call_original
+        expect(domain).to receive(:state).at_least(1).
-        expect(env).to receive(:[]).with(:force_halt).and_return(true)
+          and_return('running')
+        allow(domain).to receive(:poweroff)
       end
 
-      it "should just call poweroff" do
+      it "should poweroff" do
         expect(domain).to receive(:poweroff)
-        expect(subject.call(env)).to be_nil
+        subject.call(env)
+      end
+
+      it "should print halting message" do
+        expect(ui).to receive(:info).with('Halting domain...')
+        subject.call(env)
       end
     end
   end

spec/unit/action/shutdown_domain_spec.rb

@@ -0,0 +1,123 @@
+require 'spec_helper'
+require 'support/sharedcontext'
+require 'support/libvirt_context'
+require 'vagrant-libvirt/action/shutdown_domain'
+
+describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
+  subject { described_class.new(app, env, target_state, current_state) }
+
+  include_context 'unit'
+  include_context 'libvirt'
+
+  let(:libvirt_domain) { double('libvirt_domain') }
+  let(:servers) { double('servers') }
+  let(:current_state) { :running }
+  let(:target_state) { :shutoff }
+
+  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') }
+
+      it "should not shutdown" do
+        expect(domain).not_to receive(:shutoff)
+        subject.call(env)
+      end
+
+      it "should not print shutdown message" do
+        expect(ui).not_to receive(:info)
+        subject.call(env)
+      end
+
+      it "should provide a true result" do
+        subject.call(env)
+        expect(env[:result]).to be_truthy
+      end
+    end
+
+    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)
+      end
+
+      it "should shutdown" do
+        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)
+        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
+        end
+
+        it "should provide a false result" do
+          subject.call(env)
+          expect(env[:result]).to be_falsey
+        end
+      end
+
+      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
+        end
+
+        it "should provide a true result" do
+          subject.call(env)
+          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