diff --git a/lib/vagrant-libvirt/action.rb b/lib/vagrant-libvirt/action.rb index c9b726c..e8055ec 100644 --- a/lib/vagrant-libvirt/action.rb +++ b/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 HaltDomain + 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 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') diff --git a/lib/vagrant-libvirt/action/halt_domain.rb b/lib/vagrant-libvirt/action/halt_domain.rb index 57df2bf..117ee0b 100644 --- a/lib/vagrant-libvirt/action/halt_domain.rb +++ b/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[: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.') + if env[:machine].state.id == :running + env[:ui].info(I18n.t('vagrant_libvirt.halt_domain')) domain.poweroff end diff --git a/lib/vagrant-libvirt/action/shutdown_domain.rb b/lib/vagrant-libvirt/action/shutdown_domain.rb new file mode 100644 index 0000000..b44a254 --- /dev/null +++ b/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 diff --git a/locales/en.yml b/locales/en.yml index 5daa320..4998755 100644 --- a/locales/en.yml +++ b/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: |- diff --git a/spec/unit/action/halt_domain_spec.rb b/spec/unit/action/halt_domain_spec.rb index e229003..24f9866 100644 --- a/spec/unit/action/halt_domain_spec.rb +++ b/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 - expect(ui).to receive(:info).with('Halting domain...') + allow(ui).to receive(:info).with('Halting domain...') end - context 'with graceful timeout' do - it "should shutdown" do - expect(guest).to receive(:capability).with(:halt).and_return(true) - expect(domain).to receive(:wait_for).with(60).and_return(false) - expect(subject.call(env)).to be_nil + context "when state is not running" do + before { expect(domain).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) + subject.call(env) end - context 'when halt fails' do - before do - expect(logger).to receive(:info).with('Trying Libvirt graceful shutdown.') - 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 + it "should not print halting message" do + expect(ui).not_to receive(:info) + subject.call(env) end end - context 'with force halt enabled' do + context "when state is running" do before do - allow(env).to receive(:[]).and_call_original - expect(env).to receive(:[]).with(:force_halt).and_return(true) + expect(domain).to receive(:state).at_least(1). + 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 diff --git a/spec/unit/action/shutdown_domain_spec.rb b/spec/unit/action/shutdown_domain_spec.rb new file mode 100644 index 0000000..32bb7d7 --- /dev/null +++ b/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 diff --git a/spec/unit/action_spec.rb b/spec/unit/action_spec.rb new file mode 100644 index 0000000..92c30be --- /dev/null +++ b/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