Ensure domain removal/halt limited to provider error (#1420)

Switch to dedicated actions to trigger destroy/halt via the recovery
mechanism if the VM bring up has failed to complete the bring up of the
machine.

Fixes: #1328

lib/vagrant-libvirt/action.rb

@@ -24,6 +24,8 @@ module VagrantPlugins
           b.use ConfigValidate
           b.use BoxCheckOutdated
           b.use Call, IsCreated do |env, b2|
+            b2.use CleanupOnFailure
+
             # Create VM if not yet created.
             if !env[:result]
               b2.use SetNameOfDomain
@@ -33,6 +35,8 @@ module VagrantPlugins
                 b2.use CreateNetworkInterfaces
 
                 b2.use action_start
+
+                b2.use SetupComplete
               else
                 b2.use HandleStoragePool
                 b2.use HandleBox
@@ -51,6 +55,9 @@ module VagrantPlugins
               b2.use CreateNetworks
               b2.use action_start
             end
+
+            # corresponding action to CleanupOnFailure
+            b2.use SetupComplete
           end
         end
       end
@@ -327,6 +334,8 @@ module VagrantPlugins
 
       action_root = Pathname.new(File.expand_path('../action', __FILE__))
       autoload :PackageDomain, action_root.join('package_domain')
+      autoload :CleanupOnFailure, action_root.join('cleanup_on_failure')
+      autoload :SetupComplete, action_root.join('cleanup_on_failure')
       autoload :CreateDomain, action_root.join('create_domain')
       autoload :CreateDomainVolume, action_root.join('create_domain_volume')
       autoload :CreateNetworkInterfaces, action_root.join('create_network_interfaces')

lib/vagrant-libvirt/action/cleanup_on_failure.rb

@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+
+module VagrantPlugins
+  module ProviderLibvirt
+    module Action
+      class CleanupOnFailure
+        def initialize(app, _env)
+          @logger = Log4r::Logger.new('vagrant_libvirt::action::cleanup_on_failure')
+          @app = app
+        end
+
+        def call(env)
+          env['vagrant-libvirt.provider'] = :starting
+          @app.call(env)
+        end
+
+        def recover(env)
+          return unless env[:machine] && env[:machine].state.id != :not_created
+
+          # only destroy if failed to complete bring up
+          if env['vagrant-libvirt.provider'] == :finished
+            @logger.info("VM completed provider setup, no need to teardown")
+            return
+          end
+
+          # If we're not supposed to destroy on error then just return
+          return unless env[:destroy_on_error]
+
+          if env[:halt_on_error]
+            halt_env = env.dup
+            halt_env.delete(:interrupted)
+            halt_env[:config_validate] = false
+            env[:action_runner].run(Action.action_halt, halt_env)
+          else
+            destroy_env = env.dup
+            destroy_env.delete(:interrupted)
+            destroy_env[:config_validate] = false
+            destroy_env[:force_confirm_destroy] = true
+            env[:action_runner].run(Action.action_destroy, destroy_env)
+          end
+        end
+      end
+
+      class SetupComplete
+        def initialize(app, _env)
+          @logger = Log4r::Logger.new('vagrant_libvirt::action::cleanup_on_failure')
+          @app = app
+        end
+
+        def call(env)
+          if env['vagrant-libvirt.provider'].nil?
+            raise Errors::CallChainError, require_action: CleanupOnFailure.name, current_action: SetupComplete.name
+          end
+
+          # mark provider as finished setup so that any failure after this
+          # point doesn't result in destroying or shutting down the VM
+          env['vagrant-libvirt.provider'] = :finished
+
+          @app.call(env)
+        end
+      end
+    end
+  end
+end

lib/vagrant-libvirt/action/wait_till_up.rb

@@ -50,31 +50,6 @@ module VagrantPlugins
 
           @app.call(env)
         end
-
-        def recover(env)
-          # Undo the import
-          terminate(env)
-        end
-
-        def terminate(env)
-          if env[:machine].state.id != :not_created
-            # If we're not supposed to destroy on error then just return
-            return unless env[:destroy_on_error]
-
-            if env[:halt_on_error]
-              halt_env = env.dup
-              halt_env.delete(:interrupted)
-              halt_env[:config_validate] = false
-              env[:action_runner].run(Action.action_halt, halt_env)
-            else
-              destroy_env = env.dup
-              destroy_env.delete(:interrupted)
-              destroy_env[:config_validate] = false
-              destroy_env[:force_confirm_destroy] = true
-              env[:action_runner].run(Action.action_destroy, destroy_env)
-            end
-          end
-        end
       end
     end
   end

spec/unit/action/cleanup_on_failure_spec.rb

@@ -0,0 +1,130 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'support/sharedcontext'
+
+require 'vagrant/action/runner'
+
+require 'vagrant-libvirt/action'
+require 'vagrant-libvirt/action/cleanup_on_failure'
+
+
+describe VagrantPlugins::ProviderLibvirt::Action::CleanupOnFailure do
+  subject { described_class }
+  let(:callable_error) do
+    Class.new do
+      def initialize(app, env)
+      end
+
+      def self.name
+        "TestAction"
+      end
+
+      def call(env)
+        raise Exception, "some action failed"
+      end
+    end
+  end
+
+  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(:error)
+
+    allow(runner).to receive(:run).and_call_original
+    env[:action_runner] = runner
+    env[:destroy_on_error] = true
+  end
+
+  describe '#recover' do
+    let(:action_chain) do
+      Vagrant::Action::Builder.new.tap do |b|
+        b.use subject
+        b.use callable_error
+      end
+    end
+
+    context 'not created' do
+      before do
+        expect(state).to receive(:id).and_return(:not_created)
+      end
+
+      it 'should return early' do
+        expect(logger).to_not receive(:info).with('VM completed provider setup, no need to teardown')
+
+        expect { runner.run(action_chain) }.to raise_error(Exception, "some action failed")
+      end
+    end
+
+    context 'running' do
+      before do
+        allow(state).to receive(:id).and_return(:running)
+      end
+
+      it 'should destroy' do
+        expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_halt)
+        expect(VagrantPlugins::ProviderLibvirt::Action).to receive(:action_destroy).and_return(Vagrant::Action::Builder.new)
+        expect(logger).to_not receive(:info).with('VM completed provider setup, no need to teardown')
+
+        expect { runner.run(action_chain) }.to raise_error(Exception, "some action failed")
+      end
+
+      context 'halt on error enabled' do
+        before do
+          env[:halt_on_error] = true
+        end
+
+        it 'should halt' do
+          expect(VagrantPlugins::ProviderLibvirt::Action).to receive(:action_halt).and_return(Vagrant::Action::Builder.new)
+          expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_destroy)
+          expect(logger).to_not receive(:info).with('VM completed provider setup, no need to teardown')
+
+          expect { runner.run(action_chain) }.to raise_error(Exception, "some action failed")
+        end
+      end
+
+      context 'destroy on error disabled' do
+        before do
+          env[:destroy_on_error] = false
+        end
+
+        it 'should not destroy' do
+          expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_halt)
+          expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_destroy)
+          expect(logger).to_not receive(:info).with('VM completed provider setup, no need to teardown')
+
+          expect { runner.run(action_chain) }.to raise_error(Exception, "some action failed")
+        end
+      end
+
+      context 'completed setup' do
+        let(:action_chain) do
+          Vagrant::Action::Builder.new.tap do |b|
+            b.use subject
+            b.use VagrantPlugins::ProviderLibvirt::Action::SetupComplete
+            b.use callable_error
+          end
+        end
+
+        it 'should not perform halt or destroy' do
+          expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_halt)
+          expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_destroy)
+          expect(logger).to receive(:info).with('VM completed provider setup, no need to teardown')
+
+          expect { runner.run(action_chain) }.to raise_error(Exception, "some action failed")
+        end
+      end
+    end
+  end
+end

spec/unit/action/wait_till_up_spec.rb

@@ -86,46 +86,4 @@ describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do
       end
     end
   end
-
-  describe '#recover' do
-    before do
-      allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:get_domain).and_return(machine)
-      allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:state)
-        .and_return(:not_created)
-      allow(env).to receive(:[]).and_call_original
-    end
-
-    it 'should do nothing by default' do
-      expect(env).to_not receive(:[]).with(:action_runner) # cleanup
-      expect(subject.recover(env)).to be_nil
-    end
-
-    context 'with machine coming up' do
-      before do
-        allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:state)
-          .and_return(:running)
-        env[:destroy_on_error] = true
-      end
-
-      context 'and user has disabled destroy on failure' do
-        before do
-          env[:destroy_on_error] = false
-        end
-
-        it 'skips terminate on failure' do
-          expect(env).to_not receive(:[]).with(:action_runner) # cleanup
-          expect(subject.recover(env)).to be_nil
-        end
-      end
-
-      context 'and using default settings' do
-        let(:runner) { double('runner') }
-        it 'deletes VM on failure' do
-          expect(env).to receive(:[]).with(:action_runner).and_return(runner) # cleanup
-          expect(runner).to receive(:run)
-          expect(subject.recover(env)).to be_nil
-        end
-      end
-    end
-  end
 end