Use named pipe to decrease local Windows runtime #220

jerryaldrichiii · 2017-11-21T19:41:32Z

This uses PowerShell to spawn a process that listens on a named pipe for
Base64 encoded commands and executes them. This drastically decreases
the total runtime when running locally on Windows by using a single
PowerShell session to execute commands instead of spawning a new session
for each command.

Huge thanks to @sdelano for the initial idea!

Signed-off-by: Jerry Aldrich jerryaldrichiii@gmail.com

chris-rock · 2017-11-22T18:50:09Z

lib/train/transports/local.rb

-        res.run_command
-        CommandResult.new(res.stdout, res.stderr, res.exitstatus)
+        if defined?(@platform) && @platform.windows?
+          start_named_pipe_server unless File.exist?('//localhost/pipe/InSpec')


As highlighted by @adamleff we should make the pipe unique

jerryaldrichiii · 2017-11-22T20:23:35Z

Good catch @chris-rock @adamleff. I have fixed this to append a hex string to the end of the pipe name.

chris-rock · 2017-11-22T20:41:15Z

lib/train/transports/local.rb

+        # temporarily unavailable.
+        100.times do
+          begin
+            pipe = open(pipe_location, 'r+')


Do we need to close the pipe?

Interesting idea...after consulting with @jquick I think we should be able to do that. I'll get to work!

chris-rock · 2017-11-22T20:41:56Z

lib/train/transports/local.rb

+      end
+
+      def start_named_pipe_server(pipe_name) # rubocop:disable Metrics/MethodLength
+        require 'win32/process'


Can we implement a fallback for windows platforms where this is not available, eg. windows nano?, in this case we have powershell available already.

Is there a way I can identify Nano with current Train? If not I can fallback to not creating a pipe if I can't open one. Thoughts?

Changes are required

chris-rock

@jerryaldrichiii I think we get closer and closer. Great work. I added some questions.

chris-rock · 2017-11-23T08:23:45Z

lib/train/transports/local.rb

+        @pipe = acquire_named_pipe if @platform.windows?
+      end
+
+      def acquire_named_pipe


I think that should be a private function

chris-rock · 2017-11-23T08:26:09Z

lib/train/transports/local.rb

+        if defined?(@pipe)
+          res = run_powershell_using_named_pipe(cmd)
+          CommandResult.new(res['stdout'], res['stderr'], res['exitstatus'])
+        else


I think this is problematic, we check if a pipe is available. Lets assume we are on a windows platform and pipe creation failed, then we need to implement a fallback. Essentially we need to decide:

if os.windows? if pipe # use pipe else # fallback, which may be slower but that is okay end else # linux / mac / unix end

I'll bring back the old functionality and then wrap acquire_named_pipe in a begin/rescue. That way if the pipe fails for whatever reason, it will default to the way things run now.

chris-rock · 2017-11-23T08:26:58Z

lib/train/transports/local.rb

@@ -36,6 +67,60 @@ def local?
        true
      end

+      def run_powershell_using_named_pipe(script)


run and start pipe need to be private functions

chris-rock · 2017-11-23T08:30:13Z

lib/train/transports/local.rb

+      end
+
+      def start_named_pipe_server(pipe_name)
+        require 'win32/process'


Is there a reason why we are not using mixlib-shellout, since we just start a powershell command?

I'm using win32/process to spawn a background process. Can I use mixlib-shellout to start a background process?

Since the PowerShell server script never returns (essentially a daemon) I'm not sure I can use mixlib-shellout.

chris-rock

Thank you for the great fallback implementation @jerryaldrichiii I proposed some further improvements to make the code even more clean.

chris-rock · 2017-11-27T09:48:23Z

lib/train/transports/local.rb

-          else
-            Train::File::Local::Unix.new(self, path)
-          end
+        @files[path] ||= if os.windows?


This is rewritten by Jared, i would just keep the original to avoid merge conflicts

chris-rock · 2017-11-27T09:49:46Z

lib/train/transports/local.rb

@@ -21,13 +21,19 @@ def initialize(options)
        super(options)
        @cmd_wrapper = nil
        @cmd_wrapper = CommandWrapper.load(self, options)
+        @platform = platform


I think should stick to os or platform, but not both since they are referring to the same object.

chris-rock · 2017-11-27T10:21:34Z

lib/train/transports/local.rb

-        res.run_command
-        CommandResult.new(res.stdout, res.stderr, res.exitstatus)
+        if defined?(@platform) && @platform.windows?
+          @windows_runner ||= WindowsRunner.new


The WindowsRunner is a great step in the right direction. Should we do this analog to our file handling? eg.

Train::Command::Local::UnixRunner.new() Train::Command::Local::WindowsPipeRunner.new() # Fallback when the pipe is not working Train::Command::Local::WindowsShellRunner.new()

I like the idea of separating the Windows runners. I don't see a reason for a UnixRunner at this time. I may create a GenericRunner which will use ShellOut and be used by Linux and the first few commands that determine the OS.

The unixrunner was the default shell out :-)

Yup, but a few commands are ran using ShellOut while the OS is being identified, that's why I opted for GenericRunner.

chris-rock · 2017-11-27T10:25:53Z

lib/train/transports/local.rb

+        end
+
+        def run_command(cmd)
+          res = @pipe ? run_via_pipe(cmd) : run_via_shellout(cmd)


I would split the runner into two, so that we have a WindowsPipeRunner and a WindowsShellRunner. This would make the implementation more clean. It also allows us later to run the pipe only on supported windows systems.

chris-rock · 2017-11-27T10:26:28Z

lib/train/transports/local.rb

+        require 'securerandom'
+
+        def initialize
+          @pipe = acquire_pipe


I would just throw a train exception that can be handled and we have a proper fallback, which would be the WindowsShellRunner

Agreed! I make it so in the latest commits.

So much cleaner now, thank you!

chris-rock · 2017-11-27T14:44:43Z

lib/train/transports/local.rb

+          @windows_runner ||= WindowsRunner.new
+          @windows_runner.run_command(cmd)
+        else
+          cmd = @cmd_wrapper.run(cmd) unless @cmd_wrapper.nil?


Should we move the command wrapper handling into the GenericRunner now?

chris-rock · 2017-11-27T14:46:07Z

lib/train/transports/local.rb

-        res.run_command
-        CommandResult.new(res.stdout, res.stderr, res.exitstatus)
+        if defined?(@os) && @os.windows?
+          @windows_runner ||= WindowsRunner.new


should we decide for the runner during initialization? we could just can @runner.run_command(cmd) then here

chris-rock · 2017-11-27T14:47:41Z

lib/train/transports/local.rb

+        end
+      end
+
+      class WindowsRunner


if we move the creation to the initialize phase of Connection, I think we do not need the WindowsRunner abstraction.

chris-rock · 2017-11-27T14:48:49Z

lib/train/transports/local.rb

+        require 'securerandom'
+
+        def initialize
+          @pipe = acquire_pipe


So much cleaner now, thank you!

@sdelano

This uses PowerShell to spawn a process that listens on a named pipe for Base64 encoded commands and executes them. This drastically decreases the total runtime when running locally on Windows by using a single PowerShell session to execute commands instead of spawning a new session for each command. Huge thanks to @sdelano for the initial idea! Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

chris-rock

Thank you @jerryaldrichiii That looks good to me

adamleff

@jerryaldrichiii this is pretty great, thanks for the good work on this.

I'm a little concerned about a potential race condition, further details below.

adamleff · 2017-11-27T20:22:52Z

lib/train/transports/local.rb

+        private
+
+        def acquire_pipe
+          current_pipe = Dir.entries('//./pipe/').find { |f| f =~ /inspec_/ }


I'm a little concerned about this logic. What if there's another InSpec running on the host at the same time this one attempts to acquire a pipe? What if there's two? We could potentially find a pipe that's not ours and start using it only to have it disappear between the time we've acquired it and the time we go to use it.

Maybe my lack of knowledge around windows and named pipes is misleading my judgment here... but wouldn't it be better if we stored the name of the pipe we intend to use, look for it when we want to use it, and then if it's not there or not usable for some reason, create it? Some half-baked code to illustrate my idea...

def pipe_name @pipe_name ||= "inspec_#{SecureRandom.hex}" end def reset_pipe_name @pipe_name = nil end def acquire_pipe pipe = open("//./pipe/#{pipe_name}", ...) rescue create_pipe end def create_pipe # don't try to create a pipe of the same name if it exists but we can't open it reset_pipe_name if File.exist?(pipe_name) # do the creation and stuff, return nil if we can't create end

I don't like the logic either.

Good news, while trying to implement your solution I realized something. We don't need to open an existing pipe! The acquire_pipe method only gets called once per object so there is no need to attempt to open an existing pipe. The pipe shouldn't be shared between separate processes in any scenario.

I'll get that fixed.

A pipe is opened only once per object, multiple processes shouldn't share the same pipe because it could be terminated by the other process. Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff

@jerryaldrichiii the implementation of this looks good, however I have questions about the tests.

adamleff · 2017-11-30T16:18:41Z

test/windows/local_test.rb

@@ -40,6 +40,33 @@
    cmd.stderr.must_equal ''
  end

+  it 'uses a named pipe if available' do


I don't see anything obvious in this test that actually tests that the named pipe is used. How are you asserting that in this test?

Great point, I added words to the comments to make it more clear. Basically, it runs a PowerShell command and checks if the pipe was created by looking for the file that would be created.

adamleff · 2017-11-30T16:19:09Z

test/windows/local_test.rb

+    cmd.stderr.must_equal ''
+  end
+
+  it 'when named pipe is not available it runs `Mixlib::Shellout`' do


I don't see anything obvious in this test that actually tests that Mixlib::ShellOut is used instead of the named pipe. How are you asserting that in this test?

Great point, I added words to the comments to make it more clear. Basically, it runs a PowerShell command and that command checks to see if a pipe exists. If it does then we know it used the pipe and not Mixlib::ShellOut.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Signed-off-by: Jared Quick <jquick@chef.io>

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff

@jerryaldrichiii the tests here look much better and much easier to understand. Just a couple of cleanup items.

adamleff · 2017-12-04T17:16:08Z

test/unit/transports/local_test.rb

+
+  describe 'when running on Windows' do
+    let(:connection) { TransportHelper.new(:windows).transport.connection }
+


Remove extra newline - it's more common to group all the lets together than space them out.

adamleff · 2017-12-04T17:27:24Z

test/unit/transports/local_test.rb

    plat.add_platform_methods
+    plat.stubs(:windows?).returns(true) if type == :windows


The seems a bit brute-force. Can we accept an options hash and then we can plumb the family in as necessary and let the Train::Platforms stuff work normally without stubbing methods on this?

def initialize(opts = {}) Train::Platforms::Detect::Specifications::OS.load plat = Train::Platforms.name('mock') plat.in_family(opts[:family]) unless opts[:family].nil? plat.add_platform_methods end

Would that work?

Sure will! Great idea. See latest commit.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff

One possible improvement on the family hierarchy data type, but otherwise, this looks good to me.

adamleff · 2017-12-05T03:50:55Z

test/unit/transports/local_test.rb

    Train::Platforms::Detect::Specifications::OS.load
-    plat = Train::Platforms.name('mock')
+    plat = Train::Platforms.name(opts[:platform_name])
+    plat.family_hierarchy = opts[:family_hierarchy]


Does plat.family_hierarchy need to be an array? The default you have set makes me believe that maybe that's true...

If so, can I suggest changing this to:

plat.family_hierarchy = Array(opts[:family_hierarchy])

... so that it's always an array, even if a future test author supplies a single family string?

Based on a grep I did I see it defined as an array, but on the mock transport I see we do a flatten.

https://github.com/chef/train/blob/master/lib/train/transports/mock.rb#L80

I think I'll do what you said, but leave my examples as single item arrays.

Thoughts?

jerryaldrichiii requested a review from a team November 21, 2017 19:41

jerryaldrichiii force-pushed the ja/add-windows-pipe-server branch 7 times, most recently from 3fe7b29 to f2ecf92 Compare November 21, 2017 22:41

jerryaldrichiii changed the title ~~WIP: Use named pipe to decrease local Windows runtime~~ Use named pipe to decrease local Windows runtime Nov 21, 2017

chris-rock reviewed Nov 22, 2017

View reviewed changes

chris-rock previously approved these changes Nov 22, 2017

View reviewed changes

jerryaldrichiii force-pushed the ja/add-windows-pipe-server branch from 24946a1 to 140ac78 Compare November 22, 2017 22:32

chris-rock reviewed Nov 23, 2017

View reviewed changes

jerryaldrichiii force-pushed the ja/add-windows-pipe-server branch from 6c08b1f to b6a1d51 Compare November 27, 2017 01:15

chris-rock reviewed Nov 27, 2017

View reviewed changes

jerryaldrichiii added 12 commits November 27, 2017 12:16

Modify named pipe creation to use unique name

4540c4b

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Remove $ProgressPreference = 'SilentlyContinue'

48fa142

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Modify code to persist pipe usage

bed61c5

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Removed pipe argument (default is In/Out)

fe773e6

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Privatize pipe related functions

ccd1a8d

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Add fallback to not used pipe if creation fails

8f2437b

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Move Windows local command logic to separate class

82ffa18

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Revert file code format

776b8da

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Change @platform to @os for consistency

44a7a6a

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Split Windows runners and create a GenericRunner

9681fe6

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Add test for Windows named pipe usage

d1ad987

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii added 4 commits November 27, 2017 12:18

Tweak GenericRunner and default run_command

18a4245

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Remove Local:: from CommandWrapper

d3b6f86

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Fix test for Windows named pipe

bd2c7c3

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Make changes to integrate @jquick's cacheing magic

e7d7d32

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii force-pushed the ja/add-windows-pipe-server branch from 3f7c18e to e7d7d32 Compare November 27, 2017 18:24

chris-rock approved these changes Nov 27, 2017

View reviewed changes

adamleff suggested changes Nov 27, 2017

View reviewed changes

Remove unneeded logic for opening an existing pipe

ecad39f

A pipe is opened only once per object, multiple processes shouldn't share the same pipe because it could be terminated by the other process. Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff reviewed Nov 30, 2017

View reviewed changes

jerryaldrichiii added 2 commits November 30, 2017 11:34

Add comments explaining pipe check tests

7517045

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Change Shellout to ShellOut

d494df2

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii force-pushed the ja/add-windows-pipe-server branch from f3594f5 to dd5cdde Compare November 30, 2017 17:59

Change stubs to expects where appropriate

ab34ccc

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii force-pushed the ja/add-windows-pipe-server branch from dd5cdde to ab34ccc Compare November 30, 2017 18:02

Modify pipe tests

824f2e3

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii force-pushed the ja/add-windows-pipe-server branch from a8071fb to 824f2e3 Compare November 30, 2017 20:47

chris-rock mentioned this pull request Dec 1, 2017

Activate inspec cache by default to boost Windows execution chef-boneyard/audit#296

Closed

jerryaldrichiii and others added 3 commits December 1, 2017 17:25

Move/Modify Unit tests and add Integration tests

9965ef8

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Fix local tests when running with others.

e63627f

Signed-off-by: Jared Quick <jquick@chef.io>

Minor style changes

17d9de7

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff suggested changes Dec 4, 2017

View reviewed changes

jerryaldrichiii added 2 commits December 4, 2017 12:18

Remove extra newline between lets

6160bde

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Modify TransportHelper to accept platform options

0bfd1c1

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff approved these changes Dec 5, 2017

View reviewed changes

adamleff merged commit 9b8fc69 into master Dec 5, 2017

adamleff deleted the ja/add-windows-pipe-server branch December 5, 2017 03:56

This was referenced Apr 25, 2018

Windows local transport doesn't provide command's stderr and exit code inspec/inspec#2993

Open

Windows local transport doesn't provide command's stderr and exit code #288

Open

jerryaldrichiii mentioned this pull request Feb 27, 2019

Certain commands failing due to STDOUT being parsed by PowerShell #413

Closed

jerryaldrichiii mentioned this pull request Jul 26, 2019

Windows Inspec Train powershell error #475

Closed


		describe 'when running on Windows' do
		let(:connection) { TransportHelper.new(:windows).transport.connection }

		plat.add_platform_methods
		plat.stubs(:windows?).returns(true) if type == :windows

Use named pipe to decrease local Windows runtime #220

Use named pipe to decrease local Windows runtime #220

Conversation

jerryaldrichiii commented Nov 21, 2017

Choose a reason for hiding this comment

jerryaldrichiii commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment