From 551c303e4177ed2ec40d899df0003eeb9ba0036a Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Mon, 11 Oct 2021 19:47:40 +0100 Subject: [PATCH] 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 | 21 ++++++------ lib/vagrant-libvirt/action/shutdown_domain.rb | 34 +++++++++++++++---- lib/vagrant-libvirt/errors.rb | 4 +++ locales/en.yml | 1 + spec/unit/action/shutdown_domain_spec.rb | 33 ++++++++++++++++-- 5 files changed, 74 insertions(+), 19 deletions(-) diff --git a/lib/vagrant-libvirt/action.rb b/lib/vagrant-libvirt/action.rb index e8055ec..d7d93a0 100644 --- a/lib/vagrant-libvirt/action.rb +++ b/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| - # 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 + b3.use StartShutdownTimer + 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 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') diff --git a/lib/vagrant-libvirt/action/shutdown_domain.rb b/lib/vagrant-libvirt/action/shutdown_domain.rb index b44a254..05d3775 100644 --- a/lib/vagrant-libvirt/action/shutdown_domain.rb +++ b/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 - # handle shutdown via client capabilities - @app.call(env) + if start_time.nil? + # this really shouldn't happen + 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 diff --git a/lib/vagrant-libvirt/errors.rb b/lib/vagrant-libvirt/errors.rb index 8023755..c3a08fc 100644 --- a/lib/vagrant-libvirt/errors.rb +++ b/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) diff --git a/locales/en.yml b/locales/en.yml index 4998755..0b1786e 100644 --- a/locales/en.yml +++ b/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 diff --git a/spec/unit/action/shutdown_domain_spec.rb b/spec/unit/action/shutdown_domain_spec.rb index 7997455..7ca8357 100644 --- a/spec/unit/action/shutdown_domain_spec.rb +++ b/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