From 32f69c53610209c8b776ac1af4ff993547952b99 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Fri, 11 Jun 2021 16:04:10 -0700 Subject: [PATCH 1/6] Update HaltDomain action to only forcibly halt domain --- lib/vagrant-libvirt/action/halt_domain.rb | 36 ++--------------------- 1 file changed, 2 insertions(+), 34 deletions(-) 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 From 64d087d2f90c520f9d33948685e8481187ba5591 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Fri, 11 Jun 2021 16:04:43 -0700 Subject: [PATCH 2/6] Use builtin GracefulHalt action on halt Updates the halt action to use the GracefulHalt builtin action. If the GracefulHalt builtin action fails to properly transition the state of the guest, it will use the HaltDomain action to forcibly stop it. --- lib/vagrant-libvirt/action.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/vagrant-libvirt/action.rb b/lib/vagrant-libvirt/action.rb index c9b726c..32be744 100644 --- a/lib/vagrant-libvirt/action.rb +++ b/lib/vagrant-libvirt/action.rb @@ -143,7 +143,11 @@ module VagrantPlugins next unless env2[:result] # VM is running, halt it. - b3.use HaltDomain + b3.use Call, GracefulHalt, :shutoff, :running do |env3, b4| + if !env3[:result] + b4.use HaltDomain + end + end end end end From 44d7c5526dbe131f869b89c9fa4c787a73850b46 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Fri, 11 Jun 2021 16:06:09 -0700 Subject: [PATCH 3/6] Update HaltDomain spec to cover updated implementation --- spec/unit/action/halt_domain_spec.rb | 77 ++++++++-------------------- 1 file changed, 22 insertions(+), 55 deletions(-) diff --git a/spec/unit/action/halt_domain_spec.rb b/spec/unit/action/halt_domain_spec.rb index e229003..489e6cb 100644 --- a/spec/unit/action/halt_domain_spec.rb +++ b/spec/unit/action/halt_domain_spec.rb @@ -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 From 9acfd16dfcb8d18bb1825a7f2cdd75d9d3b976c1 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 14 Jun 2021 09:04:54 -0700 Subject: [PATCH 4/6] Add ShutdownDomain action and use during halt sequence This adds in a ShutdownDomain action which allows for the GracefulHalt action to attempt to shutdown the domain. If it does not transition to domain successfully to a shutoff state, the ShutdownDomain action is used to "nicely" shutdown the domain. Likewise, if that action fails to transition the domain, the HaltDomain action will be used to forcibly stop it. --- lib/vagrant-libvirt/action.rb | 7 ++++- lib/vagrant-libvirt/action/shutdown_domain.rb | 31 +++++++++++++++++++ locales/en.yml | 2 ++ 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 lib/vagrant-libvirt/action/shutdown_domain.rb diff --git a/lib/vagrant-libvirt/action.rb b/lib/vagrant-libvirt/action.rb index 32be744..dd9db73 100644 --- a/lib/vagrant-libvirt/action.rb +++ b/lib/vagrant-libvirt/action.rb @@ -145,7 +145,11 @@ module VagrantPlugins # VM is running, halt it. b3.use Call, GracefulHalt, :shutoff, :running do |env3, b4| if !env3[:result] - b4.use HaltDomain + b4.use Call, ShutdownDomain, :shutoff, :running do |env4, b5| + if !env4[:result] + b5.use HaltDomain + end + end end end end @@ -352,6 +356,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/shutdown_domain.rb b/lib/vagrant-libvirt/action/shutdown_domain.rb new file mode 100644 index 0000000..a1f8728 --- /dev/null +++ b/lib/vagrant-libvirt/action/shutdown_domain.rb @@ -0,0 +1,31 @@ +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 + 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 + + @app.call(env) + 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: |- From dd6b17ff9a038c4717c8acf418d2db02ab1f4aa3 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 14 Jun 2021 09:57:07 -0700 Subject: [PATCH 5/6] Add coverage on the ShutdownDomain action --- spec/unit/action/halt_domain_spec.rb | 2 +- spec/unit/action/shutdown_domain_spec.rb | 91 ++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 spec/unit/action/shutdown_domain_spec.rb diff --git a/spec/unit/action/halt_domain_spec.rb b/spec/unit/action/halt_domain_spec.rb index 489e6cb..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) } diff --git a/spec/unit/action/shutdown_domain_spec.rb b/spec/unit/action/shutdown_domain_spec.rb new file mode 100644 index 0000000..5df9690 --- /dev/null +++ b/spec/unit/action/shutdown_domain_spec.rb @@ -0,0 +1,91 @@ +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(4).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(3).times + expect(domain).to receive(:state).and_return('shutoff') + end + + it "should provide a true result" do + subject.call(env) + expect(env[:result]).to be_truthy + end + end + end + end +end From 2ae14214216f6e4f30ad75d333fcf9138a5d4116 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Mon, 2 Aug 2021 15:22:26 +0100 Subject: [PATCH 6/6] Ensure shutdown timeout adjusted if graceful halt fails --- lib/vagrant-libvirt/action.rb | 22 +++-- lib/vagrant-libvirt/action/shutdown_domain.rb | 22 ++++- spec/unit/action/shutdown_domain_spec.rb | 38 +++++++- spec/unit/action_spec.rb | 96 +++++++++++++++++++ 4 files changed, 164 insertions(+), 14 deletions(-) create mode 100644 spec/unit/action_spec.rb diff --git a/lib/vagrant-libvirt/action.rb b/lib/vagrant-libvirt/action.rb index dd9db73..e8055ec 100644 --- a/lib/vagrant-libvirt/action.rb +++ b/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 diff --git a/lib/vagrant-libvirt/action/shutdown_domain.rb b/lib/vagrant-libvirt/action/shutdown_domain.rb index a1f8728..b44a254 100644 --- a/lib/vagrant-libvirt/action/shutdown_domain.rb +++ b/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 diff --git a/spec/unit/action/shutdown_domain_spec.rb b/spec/unit/action/shutdown_domain_spec.rb index 5df9690..32bb7d7 100644 --- a/spec/unit/action/shutdown_domain_spec.rb +++ b/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 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