From 2186e92539873dd8f3b01f187f5f48913d073cc2 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Tue, 3 Mar 2020 16:13:13 -0800 Subject: [PATCH 1/6] Add `Hook` class to the autoload list --- lib/vagrant/action.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/vagrant/action.rb b/lib/vagrant/action.rb index 432147691..a646109c2 100644 --- a/lib/vagrant/action.rb +++ b/lib/vagrant/action.rb @@ -2,6 +2,7 @@ require 'vagrant/action/builder' module Vagrant module Action + autoload :Hook, 'vagrant/action/hook' autoload :Runner, 'vagrant/action/runner' autoload :Warden, 'vagrant/action/warden' From 2ac12cfde8e313e7e66cbef90043655b2c182fc4 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Tue, 3 Mar 2020 16:13:41 -0800 Subject: [PATCH 2/6] Add new hook lookup method and key generation helper --- lib/vagrant/plugin/v2/manager.rb | 49 ++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/lib/vagrant/plugin/v2/manager.rb b/lib/vagrant/plugin/v2/manager.rb index ce76d1fc1..ce21be1f4 100644 --- a/lib/vagrant/plugin/v2/manager.rb +++ b/lib/vagrant/plugin/v2/manager.rb @@ -28,6 +28,55 @@ module Vagrant result end + # Find all hooks that are applicable for the given key. This + # lookup does not include hooks which are defined for ALL_ACTIONS. + # Key lookups will match on either string or symbol values. The + # provided keys is broken down into multiple parts for lookups, + # which allows defining hooks with an entire namespaced name, + # or a short suffx. For example: + # + # Assume we are given an action class + # key = Vagrant::Action::Builtin::SyncedFolders + # + # The list of keys that will be checked for hooks: + # ["Vagrant::Action::Builtin::SyncedFolders", "vagrant_action_builtin_synced_folders", + # "Action::Builtin::SyncedFolders", "action_builtin_synced_folders", + # "Builtin::SyncedFolders", "builtin_synced_folders", + # "SyncedFolders", "synced_folders"] + # + # @param key [Class, String] key Key for hook lookups + # @return [Array] + def find_action_hooks(key) + result = [] + + generate_hook_keys(key).each do |k| + @registered.each do |plugin| + result += plugin.components.action_hooks[k] + result += plugin.components.action_hooks[k.to_sym] + end + end + + result + end + + # Generate all valid lookup keys for given key + # + # @param [Class, String] key Base key for generation + # @return [Array] all valid keys + def generate_hook_keys(key) + key = key.is_a?(Class) ? key.name : key.to_s + parts = key.split("::") + [].tap do |keys| + until parts.empty? + x = parts.join("::") + keys << x + y = x.gsub(/([a-z])([A-Z])/, '\1_\2').gsub('::', '_').downcase + keys << y if x != y + parts.shift + end + end + end + # This returns all the registered commands. # # @return [Registry>] From 98f0ec6ab7ff7e2338a9570530dfb0924dcac787 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Tue, 3 Mar 2020 16:14:07 -0800 Subject: [PATCH 3/6] Dynamically apply hooks to actions prior to execution Removes dynamic calls of before/after hooks and replaces it with proper lookups for hooks defined for the action to run. If hooks are found for an action, the action is placed in a new builder and the hooks are applied. The new stack is extracted, finalized, and then executed. --- lib/vagrant/action/warden.rb | 44 +++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/lib/vagrant/action/warden.rb b/lib/vagrant/action/warden.rb index 105b6f928..b50d219f1 100644 --- a/lib/vagrant/action/warden.rb +++ b/lib/vagrant/action/warden.rb @@ -48,20 +48,42 @@ module Vagrant action = @actions.shift @logger.info("Calling IN action: #{action}") - if !action.is_a?(Proc) && env[:hook] - hook_name = action.class.name.split("::").last. - gsub(/([a-z])([A-Z])/, '\1_\2').gsub('-', '_').downcase + actions = [action] + + # If this is an action class, set name to check for any defined hooks + if !action.is_a?(Proc) + hook_name = action.class.name + @logger.debug("Performing hooks lookup on #{hook_name} (Action: #{action} - class: #{action.class})") + hooks = Vagrant.plugin("2").manager.find_action_hooks(hook_name) + + if !hooks.empty? + @logger.debug("Found #{hooks.count} hooks defined for #{hook_name} (Action: #{action})") + # Create a new builder for the current action so we can + # properly apply the hooks with the correct ordering + b = Builder.new + b.stack << action + + hooks.each do |hook_proc| + hook = Hook.new.tap { |h| hook_proc.call(h) } + hook.apply(b) + end + + actions = b.stack.map { |a| finalize_action(a, @env) } + end end - env[:hook].call("before_#{hook_name}".to_sym) if hook_name - @stack.unshift(action).first.call(env) - env[:hook].call("after_#{hook_name}".to_sym) if hook_name + @logger.info("Calling BEGIN action: #{action}") + actions.each do |local_action| + @logger.info("Calling IN action: #{local_action} (root: #{action})") + @stack.unshift(local_action).first.call(env) + raise Errors::VagrantInterrupt if env[:interrupted] + @logger.info("Calling OUT action: #{local_action} (root: #{action})") + end - raise Errors::VagrantInterrupt if env[:interrupted] - @logger.info("Calling OUT action: #{action}") - rescue SystemExit - # This means that an "exit" or "abort" was called. In these cases, - # we just exit immediately. + @logger.info("Calling COMPLETE action: #{action}") + rescue SystemExit, NoMemoryError + # This means that an "exit" or "abort" was called, or we have run out + # of memory. In these cases, we just exit immediately. raise rescue Exception => e # We guard this so that the Warden only outputs this once for From 660166720be58f273d1ca6343d68f44f5792d379 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Tue, 3 Mar 2020 16:16:29 -0800 Subject: [PATCH 4/6] Update delayed execution implementation to utilized appended action hook --- .../darwin/cap/mount_vmware_shared_folder.rb | 28 +++++++++++-------- plugins/guests/darwin/plugin.rb | 5 ++++ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb b/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb index be4090aed..87952205b 100644 --- a/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb +++ b/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb @@ -4,6 +4,14 @@ module VagrantPlugins module GuestDarwin module Cap class MountVmwareSharedFolder + # Entry point for hook to called delayed actions + # which finalizing the synced folders setup on + # the guest + def self.write_apfs_firmlinks(env) + if env[:machine] && @_apfs_firmlinks[env[:machine].id] + @_apfs_firmlinks.delete(env[:machine].id).call + end + end # we seem to be unable to ask 'mount -t vmhgfs' to mount the roots # of specific shares, so instead we symlink from what is already @@ -14,6 +22,7 @@ module VagrantPlugins # Use this variable to determine which machines # have been registered with after hook @apply_firmlinks ||= Hash.new{ |h, k| h[k] = {bootstrap: false, content: []} } + @_apfs_firmlinks ||= Hash.new{ |h, k| h[k] = [] } machine.communicate.tap do |comm| # check if we are dealing with an APFS root container @@ -61,17 +70,14 @@ module VagrantPlugins # If we haven't already added our hook to apply firmlinks, do it now if @apply_firmlinks[machine.id][:content].empty? - Plugin.action_hook(:apfs_firmlinks, :after_synced_folders) do |hook| - action = proc { |*_| - content = @apply_firmlinks[machine.id][:content].join("\n") - # Write out the synthetic file - comm.sudo("echo -e #{content.inspect} > /etc/synthetic.conf") - if @apply_firmlinks[:bootstrap] - # Re-bootstrap the root container to pick up firmlink updates - comm.sudo("/System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B") - end - } - hook.prepend(action) + @_apfs_firmlinks[machine.id] = proc do + content = @apply_firmlinks[machine.id][:content].join("\n") + # Write out the synthetic file + comm.sudo("echo -e #{content.inspect} > /etc/synthetic.conf") + if @apply_firmlinks[:bootstrap] + # Re-bootstrap the root container to pick up firmlink updates + comm.sudo("/System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B") + end end end @apply_firmlinks[machine.id][:content] << share_line diff --git a/plugins/guests/darwin/plugin.rb b/plugins/guests/darwin/plugin.rb index 8cadc9f31..df910b0be 100644 --- a/plugins/guests/darwin/plugin.rb +++ b/plugins/guests/darwin/plugin.rb @@ -6,6 +6,11 @@ module VagrantPlugins name "Darwin guest" description "Darwin guest support." + action_hook(:apfs_firmlinks, :synced_folders) do |hook| + require_relative "cap/mount_vmware_shared_folder" + hook.append(Cap::MountVmwareSharedFolder.method(:write_apfs_firmlinks)) + end + guest(:darwin, :bsd) do require_relative "guest" Guest From 3686aed72dfbeae39b71a19b7d52d4515098dd7e Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Tue, 3 Mar 2020 16:16:55 -0800 Subject: [PATCH 5/6] Add test coverage for dynamic action hook support --- .../cap/mount_vmware_shared_folder_test.rb | 8 - test/unit/vagrant/action/warden_test.rb | 153 +++++++++++++++--- test/unit/vagrant/plugin/v2/manager_test.rb | 116 +++++++++++++ 3 files changed, 249 insertions(+), 28 deletions(-) diff --git a/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb b/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb index 71dfbd829..6261c4f05 100644 --- a/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb +++ b/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb @@ -39,10 +39,6 @@ describe "VagrantPlugins::GuestDarwin::Cap::MountVmwareSharedFolder" do expect(communicator).to receive(:test).with(/synthetic\.conf/) end - it "should register an action hook" do - expect(VagrantPlugins::GuestDarwin::Plugin).to receive(:action_hook).with(:apfs_firmlinks, :after_synced_folders) - end - context "with guest path within existing directory" do let(:guestpath) { "/Users/vagrant/workspace" } @@ -77,10 +73,6 @@ describe "VagrantPlugins::GuestDarwin::Cap::MountVmwareSharedFolder" do expect(communicator).to receive(:sudo).with(%r{ln -s .+/System/Volumes/Data.+}) end - it "should register an action hook" do - expect(VagrantPlugins::GuestDarwin::Plugin).to receive(:action_hook).with(:apfs_firmlinks, :after_synced_folders) - end - context "when firmlink is provided by the system" do before { expect(described_class).to receive(:system_firmlink?).and_return(true) } diff --git a/test/unit/vagrant/action/warden_test.rb b/test/unit/vagrant/action/warden_test.rb index d119f871a..439dead01 100644 --- a/test/unit/vagrant/action/warden_test.rb +++ b/test/unit/vagrant/action/warden_test.rb @@ -7,6 +7,7 @@ describe Vagrant::Action::Warden do end def call(env) + env[:data] << 1 if env[:data] @app.call(env) end @@ -21,6 +22,7 @@ describe Vagrant::Action::Warden do end def call(env) + env[:data] << 2 if env[:data] @app.call(env) end @@ -90,35 +92,146 @@ describe Vagrant::Action::Warden do expect(data.key?(:recover)).not_to be end - context "when hook is defined" do - let(:hook) { double("hook") } + it "does not do a recovery sequence if NoMemoryError is raised" do + error_proc = Proc.new { raise NoMemoryError } - before do - data[:hook] = hook - allow(hook).to receive(:call) + instance = described_class.new([ExitAction, error_proc], data) + + # The SystemExit should come through + expect { instance.call(data) }.to raise_error(NoMemoryError) + + # The recover should not have been called + expect(data.key?(:recover)).not_to be + end + + describe "dynamic action hooks" do + let(:data) { {data: []} } + let(:hook_action_name) { :action_two } + + let(:plugin) do + h_name = hook_action_name + @plugin ||= Class.new(Vagrant.plugin("2")) do + name "Test Plugin" + action_hook(:before_test, h_name) do |hook| + hook.prepend(proc{ |env| env[:data] << :first }) + end + end end - it "should receive a before hook call" do - expect(hook).to receive(:call).with(:before_action_one) - described_class.new([ActionOne], data).call(data) + before { plugin } + + after do + Vagrant.plugin("2").manager.unregister(@plugin) if @plugin + @plugin = nil end - it "should receive an after hook call" do - expect(hook).to receive(:call).with(:after_action_one) - described_class.new([ActionOne], data).call(data) + it "should call hook before running action" do + instance = described_class.new([ActionTwo], data) + instance.call(data) + expect(data[:data].first).to eq(:first) + expect(data[:data].last).to eq(2) end - it "should not receive any hook calls for proc instances" do - expect(hook).not_to receive(:call) - described_class.new([proc{|*_| :testing }], data).call(data) + context "when hook is appending to action" do + let(:plugin) do + @plugin ||= Class.new(Vagrant.plugin("2")) do + name "Test Plugin" + action_hook(:before_test, :action_two) do |hook| + hook.append(proc{ |env| env[:data] << :first }) + end + end + end + + it "should call hook after action when action is nested" do + instance = described_class.new([described_class.new([ActionTwo], data), ActionOne], data) + instance.call(data) + expect(data[:data][0]).to eq(2) + expect(data[:data][1]).to eq(:first) + expect(data[:data][2]).to eq(1) + end end - it "should receive before and after calls for each class" do - expect(hook).to receive(:call).with(:before_action_one) - expect(hook).to receive(:call).with(:after_action_one) - expect(hook).to receive(:call).with(:before_action_two) - expect(hook).to receive(:call).with(:after_action_two) - described_class.new([ActionOne, proc{|*_| :testing }, ActionTwo], data).call(data) + context "when hook uses class name" do + let(:hook_action_name) { "ActionTwo" } + + it "should execute the hook" do + instance = described_class.new([ActionTwo], data) + instance.call(data) + expect(data[:data]).to include(:first) + end + end + + context "when action includes a namespace" do + module Vagrant + module Test + class ActionTest + def initialize(app, env) + @app = app + end + + def call(env) + env[:data] << :test if env[:data] + @app.call(env) + end + end + end + end + + let(:instance) { described_class.new([Vagrant::Test::ActionTest], data) } + + context "when hook uses short snake case name" do + let(:hook_action_name) { :action_test } + + it "should execute the hook" do + instance.call(data) + expect(data[:data]).to include(:first) + end + end + + context "when hook uses partial snake case name" do + let(:hook_action_name) { :test_action_test } + + it "should execute the hook" do + instance.call(data) + expect(data[:data]).to include(:first) + end + end + + context "when hook uses full snake case name" do + let(:hook_action_name) { :vagrant_test_action_test } + + it "should execute the hook" do + instance.call(data) + expect(data[:data]).to include(:first) + end + end + + context "when hook uses short class name" do + let(:hook_action_name) { "ActionTest" } + + it "should execute the hook" do + instance.call(data) + expect(data[:data]).to include(:first) + end + end + + context "when hook uses partial namespace class name" do + let(:hook_action_name) { "Test::ActionTest" } + + it "should execute the hook" do + instance.call(data) + expect(data[:data]).to include(:first) + end + end + + context "when hook uses full namespace class name" do + let(:hook_action_name) { "Vagrant::Test::ActionTest" } + + it "should execute the hook" do + instance.call(data) + expect(data[:data]).to include(:first) + end + end end end end diff --git a/test/unit/vagrant/plugin/v2/manager_test.rb b/test/unit/vagrant/plugin/v2/manager_test.rb index 3ddbf00e9..c145b4b28 100644 --- a/test/unit/vagrant/plugin/v2/manager_test.rb +++ b/test/unit/vagrant/plugin/v2/manager_test.rb @@ -11,6 +11,122 @@ describe Vagrant::Plugin::V2::Manager do p end + describe "#generate_hook_keys" do + it "should return array with one value" do + expect(subject.generate_hook_keys(:test_value)).to eq(["test_value"]) + end + + it "should return array with two values when key is camel cased" do + result = subject.generate_hook_keys("TestValue") + expect(result.size).to eq(2) + expect(result).to include("TestValue") + expect(result).to include("test_value") + end + + it "should handle class/module value" do + result = subject.generate_hook_keys(Vagrant) + expect(result.size).to eq(2) + expect(result).to include("Vagrant") + expect(result).to include("vagrant") + end + + it "should handle namespaced value" do + result = subject.generate_hook_keys(Vagrant::Plugin) + expect(result.size).to eq(4) + expect(result).to include("Vagrant::Plugin") + expect(result).to include("Plugin") + expect(result).to include("vagrant_plugin") + expect(result).to include("plugin") + end + end + + describe "#find_action_hooks" do + let(:hook_name) { "Vagrant::Plugin" } + + before do + h_name = hook_name + pA = plugin do |p| + p.action_hook(:test, h_name) { "hook_called" } + end + subject.register(pA) + end + + it "should find hook with full namespace" do + hooks = subject.find_action_hooks(Vagrant::Plugin) + expect(hooks).not_to be_empty + expect(hooks.first.call).to eq("hook_called") + end + + it "should not find hook with short class name" do + hooks = subject.find_action_hooks("Plugin") + expect(hooks).to be_empty + end + + it "should not find hook with full snake cased name" do + hooks = subject.find_action_hooks(:vagrant_plugin) + expect(hooks).to be_empty + end + + it "should not find hook with short snake cased name" do + hooks = subject.find_action_hooks("plugin") + expect(hooks).to be_empty + end + + context "when hook uses full snake cased name" do + let(:hook_name) { :vagrant_plugin } + + it "should find hook with full namespace" do + hooks = subject.find_action_hooks(Vagrant::Plugin) + expect(hooks).not_to be_empty + expect(hooks.first.call).to eq("hook_called") + end + + it "should find hook with full snake cased name" do + hooks = subject.find_action_hooks(:vagrant_plugin) + expect(hooks).not_to be_empty + expect(hooks.first.call).to eq("hook_called") + end + + it "should not find hook with short class name" do + hooks = subject.find_action_hooks("Plugin") + expect(hooks).to be_empty + end + + it "should not find hook with short snake cased name" do + hooks = subject.find_action_hooks("plugin") + expect(hooks).to be_empty + end + end + + context "when hook uses short snake cased name" do + let(:hook_name) { :plugin } + + it "should find hook with full namespace" do + hooks = subject.find_action_hooks(Vagrant::Plugin) + expect(hooks).not_to be_empty + expect(hooks.first.call).to eq("hook_called") + end + + it "should find hook with short class name" do + hooks = subject.find_action_hooks("Plugin") + expect(hooks).not_to be_empty + expect(hooks.first.call).to eq("hook_called") + end + + it "should find hook with short snake cased name" do + hooks = subject.find_action_hooks("plugin") + expect(hooks).not_to be_empty + expect(hooks.first.call).to eq("hook_called") + end + + it "should not find hook with full snake cased name" do + hooks = subject.find_action_hooks(:vagrant_plugin) + expect(hooks).to be_empty + end + end + + end + describe "#action_hooks" do it "should contain globally registered hooks" do pA = plugin do |p| From 7c2083d5da9a1d4f38718e05e900544c63d28ba3 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 5 Mar 2020 13:25:57 -0800 Subject: [PATCH 6/6] Update how delayed actions are stored to avoid accessing uninitialized value --- .../darwin/cap/mount_vmware_shared_folder.rb | 15 ++++-- .../cap/mount_vmware_shared_folder_test.rb | 51 +++++++++++++++++++ 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb b/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb index 87952205b..29f2dac79 100644 --- a/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb +++ b/plugins/guests/darwin/cap/mount_vmware_shared_folder.rb @@ -8,11 +8,19 @@ module VagrantPlugins # which finalizing the synced folders setup on # the guest def self.write_apfs_firmlinks(env) - if env[:machine] && @_apfs_firmlinks[env[:machine].id] - @_apfs_firmlinks.delete(env[:machine].id).call + if env && env[:machine] && delayed = apfs_firmlinks_delayed.delete(env[:machine].id) + delayed.call end end + # @return [Hash] storage location for delayed actions + def self.apfs_firmlinks_delayed + if !@_apfs_firmlinks + @_apfs_firmlinks = {} + end + @_apfs_firmlinks + end + # we seem to be unable to ask 'mount -t vmhgfs' to mount the roots # of specific shares, so instead we symlink from what is already # mounted by the guest tools @@ -22,7 +30,6 @@ module VagrantPlugins # Use this variable to determine which machines # have been registered with after hook @apply_firmlinks ||= Hash.new{ |h, k| h[k] = {bootstrap: false, content: []} } - @_apfs_firmlinks ||= Hash.new{ |h, k| h[k] = [] } machine.communicate.tap do |comm| # check if we are dealing with an APFS root container @@ -70,7 +77,7 @@ module VagrantPlugins # If we haven't already added our hook to apply firmlinks, do it now if @apply_firmlinks[machine.id][:content].empty? - @_apfs_firmlinks[machine.id] = proc do + apfs_firmlinks_delayed[machine.id] = proc do content = @apply_firmlinks[machine.id][:content].join("\n") # Write out the synthetic file comm.sudo("echo -e #{content.inspect} > /etc/synthetic.conf") diff --git a/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb b/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb index 6261c4f05..a706caa21 100644 --- a/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb +++ b/test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb @@ -156,4 +156,55 @@ describe "VagrantPlugins::GuestDarwin::Cap::MountVmwareSharedFolder" do end end end + + describe ".write_apfs_firmlinks" do + let(:env) { nil } + let(:action) { double("action", call: nil) } + let(:machine) { double("machine", id: machine_id) } + let(:machine_id) { double("machine_id") } + let(:delayed) { {} } + + context "when env is nil" do + it "should be a no-op" do + expect(described_class).not_to receive(:apfs_firmlinks_delayed) + described_class.write_apfs_firmlinks(env) + end + end + + context "when env is empty hash" do + let(:env) { {} } + + it "should be a no-op" do + expect(described_class).not_to receive(:apfs_firmlinks_delayed) + described_class.write_apfs_firmlinks(env) + end + end + + context "when machine is defined within env" do + let(:env) { {machine: machine} } + + it "should request the stored delayed actions" do + expect(described_class).to receive(:apfs_firmlinks_delayed).and_return(delayed) + described_class.write_apfs_firmlinks(env) + end + end + + context "when delayed action is stored for machine" do + let(:env) { {machine: machine} } + + before { described_class.apfs_firmlinks_delayed[machine_id] = action } + after { described_class.apfs_firmlinks_delayed.clear } + + it "should call the delayed action" do + expect(action).to receive(:call) + described_class.write_apfs_firmlinks(env) + end + + it "should remove the action after calling" do + expect(action).to receive(:call) + described_class.write_apfs_firmlinks(env) + expect(delayed).to be_empty + end + end + end end