Rework shutdown domain to avoid trailing action (#1377)

Using the trailing action for ShutdownDomain results in it being
executed as part of the start up sequence in the reload action.

This appears to happen because the halt action is not executed in
isolation from the start action that is scheduled afterwards and
consequently the chaining behaviour spans both instead of being treated
as the combination of two distinct actions that should complete.

While this fixes the issue, it is important that subsequently call such
actions can be done without allowing the out part of the middleware
behaviour to be applied to subsequent actions.

Partial-Fix: #1376

lib/vagrant-libvirt/action.rb

@@ -143,19 +143,17 @@ module VagrantPlugins
             b2.use Call, IsRunning do |env2, b3|
               next unless env2[:result]
 
-              b3.use Call, Message, "Attempting nice shutdowns..." do |_, b4|
+              b3.use StartShutdownTimer
-                # ShutdownDomain will perform the domain shutdown on the out calls
+              b3.use Call, GracefulHalt, :shutoff, :running do |env3, b4|
-                # so it runs after the remaining actions in the same action builder.
+                if !env3[:result]
-                b4.use ShutdownDomain, :shutoff, :running
+                  b4.use Call, ShutdownDomain, :shutoff, :running do |env4, b5|
-                b4.use GracefulHalt, :shutoff, :running
+                    if !env4[:result]
+                       b5.use HaltDomain
+                    end
+                  end
+                end
               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
@@ -360,6 +358,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 :StartShutdownTimer, action_root.join('shutdown_domain')
       autoload :ShutdownDomain, action_root.join('shutdown_domain')
       autoload :HandleBoxImage, action_root.join('handle_box_image')
       autoload :HandleStoragePool, action_root.join('handle_storage_pool')

lib/vagrant-libvirt/action/shutdown_domain.rb

@@ -1,5 +1,24 @@
 require 'log4r'
 
+module VagrantPlugins
+  module ProviderLibvirt
+    module Action
+      # To wrap GracefulShutdown need to track the time taken
+      class StartShutdownTimer
+        def initialize(app, _env)
+          @app = app
+        end
+
+        def call(env)
+          env[:shutdown_start_time] = Time.now
+
+          @app.call(env)
+        end
+      end
+    end
+  end
+end
+
 module VagrantPlugins
   module ProviderLibvirt
     module Action
@@ -15,21 +34,22 @@ module VagrantPlugins
         def call(env)
           timeout = env[:machine].config.vm.graceful_halt_timeout
 
-          start_time = Time.now
+          start_time = env[:shutdown_start_time]
 
-          # call nested action first under the assumption it should try to
+          if start_time.nil?
-          # handle shutdown via client capabilities
+            # this really shouldn't happen
-          @app.call(env)
+            raise Errors::CallChainError, require_action: StartShutdownTimer.name, current_action: ShutdownDomain.name
+          end
 
           # return if successful, otherwise will ensure result is set to false
           env[:result] = env[:machine].state.id == @target_state
 
-          return if env[:result]
+          return @app.call(env) if env[:result]
 
           current_time = Time.now
 
           # if we've already exceeded the timeout
-          return if current_time - start_time >= timeout
+          return @app.call(env) if current_time - start_time >= timeout
 
           # otherwise construct a new timeout.
           timeout = timeout - (current_time - start_time)
@@ -42,6 +62,8 @@ module VagrantPlugins
           end
 
           env[:result] = env[:machine].state.id == @target_state
+
+          @app.call(env)
         end
       end
     end

lib/vagrant-libvirt/errors.rb

@@ -9,6 +9,10 @@ module VagrantPlugins
         error_namespace('vagrant_libvirt.errors')
       end
 
+      class CallChainError < VagrantLibvirtError
+        error_key(:call_chain_error)
+      end
+
       # package not supported
       class PackageNotSupported < VagrantLibvirtError
         error_key(:package_not_supported)

locales/en.yml

@@ -62,6 +62,7 @@ en:
         Forwarding UDP ports is not supported. Ignoring.
 
     errors:
+      call_chain_error: Invalid action chain, must ensure that '%{require_action}' is called prior to calling '%{current_action}'
       package_not_supported: No support for package with Libvirt. Create box manually.
       fog_error: |-
         There was an error talking to Libvirt. The error message is shown

spec/unit/action/shutdown_domain_spec.rb

@@ -3,6 +3,20 @@ require 'support/sharedcontext'
 require 'support/libvirt_context'
 require 'vagrant-libvirt/action/shutdown_domain'
 
+describe VagrantPlugins::ProviderLibvirt::Action::StartShutdownTimer do
+  subject { described_class.new(app, env) }
+
+  include_context 'unit'
+
+  describe '#call' do
+    it 'should set shutdown_start_time' do
+      expect(env[:shutdown_start_time]).to eq(nil)
+      expect(subject.call(env)).to eq(nil)
+      expect(env[:shutdown_start_time]).to_not eq(nil)
+    end
+  end
+end
+
 describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
   subject { described_class.new(app, env, target_state, current_state) }
 
@@ -26,6 +40,8 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
       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...')
+      allow(env).to receive(:[]).and_call_original
+      allow(env).to receive(:[]).with(:shutdown_start_time).and_return(Time.now)
     end
 
     context "when state is shutoff" do
@@ -97,7 +113,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
       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(Time).to receive(:now).and_return(env[:shutdown_start_time] + 2)
           expect(driver).to receive(:state).and_return(:running).exactly(1).times
           expect(domain).to_not receive(:wait_for)
           expect(domain).to_not receive(:shutdown)
@@ -112,7 +128,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
       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(Time).to receive(:now).and_return(env[:shutdown_start_time] + 1.5)
           expect(driver).to receive(:state).and_return(:running).exactly(3).times
           expect(domain).to receive(:wait_for) do |time|
             expect(time).to be < 1
@@ -127,5 +143,18 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
         end
       end
     end
+
+    context "when required action not run" do
+      before do
+        expect(env).to receive(:[]).with(:shutdown_start_time).and_call_original
+      end
+
+      it "should raise an exception" do
+        expect { subject.call(env) }.to raise_error(
+          VagrantPlugins::ProviderLibvirt::Errors::CallChainError,
+          /Invalid action chain, must ensure that '.*ShutdownTimer' is called prior to calling '.*ShutdownDomain'/
+        )
+      end
+    end
   end
 end