powershell and cmd calls should use commnand_executor to reuse oprn winrm shell

plugins/communicators/winrm/communicator.rb

@@ -142,6 +142,7 @@ module VagrantPlugins
 
         opts[:good_exit] = Array(opts[:good_exit])
         command = wrap_in_scheduled_task(command, opts[:interactive]) if opts[:elevated]
+        @logger.debug("#{opts[:shell]} executing:\n#{command}")
         output = shell.send(opts[:shell], command, &block)
         execution_output(output, opts)
       end

plugins/communicators/winrm/shell.rb

@@ -54,20 +54,21 @@ module VagrantPlugins
       end
 
       def powershell(command, &block)
-        # Suppress the progress stream from leaking to stderr
-        command = "$ProgressPreference='SilentlyContinue';\r\n" + command
-        command << "\r\n"
         # Ensure an exit code
-        command << "if ($?) { exit 0 } else { if($LASTEXITCODE) { exit $LASTEXITCODE } else { exit 1 } }"
+        command += "\r\nif ($?) { exit 0 } else { if($LASTEXITCODE) { exit $LASTEXITCODE } else { exit 1 } }"
-        execute_shell(command, :powershell, &block)
+        execute_with_rescue(executor.method("run_powershell_script"), command, &block)
       end
 
       def cmd(command, &block)
-        execute_shell(command, :cmd, &block)
+        execute_with_rescue(executor.method("run_cmd"), command, &block)
       end
 
       def wql(query, &block)
-        execute_shell(query, :wql, &block)
+        retryable(tries: @config.max_tries, on: @@exceptions_to_retry_on, sleep: @config.retry_delay) do
+          handle_output(session.method("run_wql"), query, &block)
+        end
+      rescue => e
+        raise_winrm_exception(e, "run_wql", query)
       end
 
       def upload(from, to)
@@ -82,42 +83,35 @@ module VagrantPlugins
 
       protected
 
-      def execute_shell(command, shell=:powershell, &block)
+      def execute_with_rescue(method, command, &block)
-        raise Errors::WinRMInvalidShell, shell: shell unless [:powershell, :cmd, :wql].include?(shell)
+        handle_output(method, command, &block)
-
+      rescue => e
-        begin
+        raise_winrm_exception(e, method.name, command)
-          execute_shell_with_retry(command, shell, &block)
-        rescue => e
-          raise_winrm_exception(e, shell, command)
-        end
       end
 
-      def execute_shell_with_retry(command, shell, &block)
+      def handle_output(execute_method, command, &block)
-        retryable(tries: @config.max_tries, on: @@exceptions_to_retry_on, sleep: @config.retry_delay) do
+        output = execute_method.call(command) do |out, err|
-          @logger.debug("#{shell} executing:\n#{command}")
+          block.call(:stdout, out) if block_given? && out
-          output = session.send(shell, command) do |out, err|
+          block.call(:stderr, err) if block_given? && err
-            block.call(:stdout, out) if block_given? && out
+        end
-            block.call(:stderr, err) if block_given? && err
-          end
 
-          @logger.debug("Output: #{output.inspect}")
+        @logger.debug("Output: #{output.inspect}")
 
-          # Verify that we didn't get a parser error, and if so we should
+        # Verify that we didn't get a parser error, and if so we should
-          # set the exit code to 1. Parse errors return exit code 0 so we
+        # set the exit code to 1. Parse errors return exit code 0 so we
-          # need to do this.
+        # need to do this.
-          if output[:exitcode] == 0
+        if output[:exitcode] == 0
-            (output[:data] || []).each do |data|
+          (output[:data] || []).each do |data|
-              next if !data[:stderr]
+            next if !data[:stderr]
-              if data[:stderr].include?("ParserError")
+            if data[:stderr].include?("ParserError")
-                @logger.warn("Detected ParserError, setting exit code to 1")
+              @logger.warn("Detected ParserError, setting exit code to 1")
-                output[:exitcode] = 1
+              output[:exitcode] = 1
-                break
+              break
-              end
             end
           end
-
-          return output
         end
+
+        return output
       end
 
       def raise_winrm_exception(exception, shell = nil, command = nil)
@@ -178,6 +172,10 @@ module VagrantPlugins
         @session ||= new_session
       end
 
+      def executor
+        @executor ||= session.create_executor
+      end
+
       def endpoint
         case @config.transport.to_sym
         when :ssl
@@ -195,7 +193,9 @@ module VagrantPlugins
           host: @host,
           port: @port,
           basic_auth_only: @config.basic_auth_only,
-          no_ssl_peer_verification: !@config.ssl_peer_verification }
+          no_ssl_peer_verification: !@config.ssl_peer_verification,
+          retry_delay: @config.retry_delay,
+          retry_limit: @config.max_tries }
       end
     end #WinShell class
   end

test/unit/plugins/communicators/winrm/shell_test.rb

@@ -6,7 +6,8 @@ require Vagrant.source_root.join("plugins/communicators/winrm/config")
 describe VagrantPlugins::CommunicatorWinRM::WinRMShell do
   include_context "unit"
 
-  let(:session) { double("winrm_session") }
+  let(:session) { double("winrm_session", create_executor: executor) }
+  let(:executor) { double("command_executor") }
   let(:port) { config.transport == :ssl ? 5986 : 5985 }
   let(:config)  {
     VagrantPlugins::CommunicatorWinRM::Config.new.tap do |c|
@@ -15,6 +16,8 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do
       c.max_tries = 3
       c.retry_delay = 0
       c.basic_auth_only = false
+      c.retry_delay = 1
+      c.max_tries = 2
       c.finalize!
     end
   }
@@ -27,23 +30,12 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do
 
   describe ".powershell" do
     it "should call winrm powershell" do
-      expect(session).to receive(:powershell).with(/^dir.+/).and_return({ exitcode: 0 })
+      expect(executor).to receive(:run_powershell_script).with(/^dir.+/).and_return({ exitcode: 0 })
       expect(subject.powershell("dir")[:exitcode]).to eq(0)
     end
 
-    it "should retry when a WinRMAuthorizationError is received" do
-      expect(session).to receive(:powershell).with(/^dir.+/).exactly(3).times.and_raise(
-        # Note: The initialize for WinRMAuthorizationError may require a status_code as
-        # the second argument in a future WinRM release. Currently it doesn't track the
-        # status code.
-        WinRM::WinRMAuthorizationError.new("Oh no!! Unauthrorized")
-      )
-      expect { subject.powershell("dir") }.to raise_error(
-        VagrantPlugins::CommunicatorWinRM::Errors::AuthenticationFailed)
-    end
-
     it "should raise an execution error when an exception occurs" do
-      expect(session).to receive(:powershell).with(/^dir.+/).and_raise(
+      expect(executor).to receive(:run_powershell_script).with(/^dir.+/).and_raise(
         StandardError.new("Oh no! a 500 SOAP error!"))
       expect { subject.powershell("dir") }.to raise_error(
         VagrantPlugins::CommunicatorWinRM::Errors::ExecutionError)
@@ -52,11 +44,29 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do
 
   describe ".cmd" do
     it "should call winrm cmd" do
-      expect(session).to receive(:cmd).with("dir").and_return({ exitcode: 0 })
+      expect(executor).to receive(:run_cmd).with("dir").and_return({ exitcode: 0 })
       expect(subject.cmd("dir")[:exitcode]).to eq(0)
     end
   end
 
+  describe ".wql" do
+    it "should call winrm wql" do
+      expect(session).to receive(:run_wql).with("select * from Win32_OperatingSystem").and_return({ exitcode: 0 })
+      expect(subject.wql("select * from Win32_OperatingSystem")[:exitcode]).to eq(0)
+    end
+
+    it "should retry when a WinRMAuthorizationError is received" do
+      expect(session).to receive(:run_wql).with("select * from Win32_OperatingSystem").exactly(2).times.and_raise(
+        # Note: The initialize for WinRMAuthorizationError may require a status_code as
+        # the second argument in a future WinRM release. Currently it doesn't track the
+        # status code.
+        WinRM::WinRMAuthorizationError.new("Oh no!! Unauthrorized")
+      )
+      expect { subject.wql("select * from Win32_OperatingSystem") }.to raise_error(
+        VagrantPlugins::CommunicatorWinRM::Errors::AuthenticationFailed)
+    end
+  end
+
   describe ".endpoint" do
     context 'when transport is :ssl' do
       let(:config)  {
@@ -93,7 +103,8 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do
     it "should create endpoint options" do
       expect(subject.send(:endpoint_options)).to eq(
         { user: "username", pass: "password", host: "localhost", port: 5985,
-          basic_auth_only: false, no_ssl_peer_verification: false })
+          basic_auth_only: false, no_ssl_peer_verification: false,
+          retry_delay: 1, retry_limit: 2 })
     end
   end