[BOLT-72] Don't invoke new powershell instances Created: 2017/09/08  Updated: 2017/11/29  Resolved: 2017/11/29

Status: Closed
Project: Puppet Task Runner
Component/s: Windows
Affects Version/s: None
Fix Version/s: BOLT 0.9.0

Type: Improvement Priority: Normal
Reporter: Josh Cooper Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: docs_reviewed
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
relates to BOLT-206 Winrm connector doesn't always propag... Closed
relates to BOLT-234 Avoid sharing runspace across powersh... Closed
relates to BOLT-208 WinRM node should run arbitrary white... Closed
Sprint: Bolt Kanban
Release Notes: New Feature
Release Notes Summary: This improves performance of the WinRM transport by running PowerShell-based Bolt scripts and tasks in the connected WinRM session, rather than creating new PowerShell processes. This also fixes an issue with properly reflecting failure status on PowerShell versions older than PowerShell 5. Users should not rely on variables set between commands persisting, even those set in the $global scope. Users should not rely on [Console]::WriteLine to produce output from PowerShell code.
QA Risk Assessment: Needs Assessment


When executing a script or task, we copy the file to the remote system over winrm, and then invoke powershell.exe on the file. Invoking powershell is slow, and we could instead eval the script contents in our current powershell instance.

However, to do that we need to make sure we clear any state from a previously executed command or script, which includes clearing PT_ environment variables. Longer term we probably need to implement runspaces like we do for the powershell module. But for this ticket, I think it's ok to just clear environment variables and no longer invoke another powershell instance.

Comment by Josh Cooper [ 2017/09/08 ]

For example, we should be able to change:

    def run_script(script)
-      with_remote_file(script) do |remote_path|
-        args = '-NoProfile -NonInteractive -NoLogo -ExecutionPolicy Bypass'
-        execute("powershell.exe #{args} -File '#{remote_path}'")
-      end
+      cmd = File.read(script)
+      execute(cmd)
+    rescue => ex
+      Bolt::ExceptionFailure.new(ex)

Comment by Ethan Brown [ 2017/11/07 ]

Note that this as currently described, this runs a bit counter to BOLT-208.

I believe that (after some research into how "isolated" script executions are given the current model), that this ticket should be about more generally optimizing the PowerShell workflow for executing scripts / tasks.

Comment by Ethan Brown [ 2017/11/07 ]

I put together a few quick tests around understanding if there is bleed across invocations, but I think some more work should be done to fully vet.

It appears that the between the PowerShell ServerRemoteHost using a RunspacePool when executing PowerShell code, and the winrm gem performing runspace management, that each command invocation is isolated already.

I ran some simple tests with Bolt over command, script and task execution, and noticed that no state was shared from one command to the next. Note that in this case, script and task were both running against new powershell.exe instances, but the behavior of command is enough to conclude that state is not shared.


So in broader terms, I think that we can move forward on simplifying the PowerShell execution mechanism so that Invoke-Interpreter is not called under the hood to launch a new powershell.exe for the sake of isolating runs. This also means that we shouldn't need anything like the runspace manager in the PowerShell module at https://github.com/puppetlabs/puppetlabs-powershell/blob/master/lib/puppet_x/templates/powershell/init_ps.ps1 as runspaces are already being managed. It seems reasonable to assume that ResetRunspaceState is already called on the Runspaces as they're handed out ... but worst case scenario we may have to perform some up-front Runspace cleanup like what's done in the PowerShell module at https://github.com/puppetlabs/puppetlabs-powershell/blob/d0166e4ec6b7e6f1abdb21f9efb439a700940ee1/lib/puppet_x/templates/powershell/init_ps.ps1#L414-L463

When this change is made, some tests should be added to show state doesn't bleed (we may have to ask the winrm library to use some well-known GUIDs to get the same runspace out of the pool)

Comment by Ethan Brown [ 2017/11/21 ]

Determined that it's still best to send the entire file over rather than doing an inline embed / eval, given some of the concessions that have to be made for that to work.

From notes on the PR

After further thought and some discussion / pairing with @michaeltlombardi, I've removed e746be7, which was an exploration into running PowerShell scripts without writing files. While it did improve performance, it came with a few downsides:

  • The PowerShell scripts themselves could not include `@'` / `'@` style here-strings, which in itself is likely a deal breaker.
  • We would have had to deal more directly with the encoding of script files on disk rather than simply passing them to remote hosts to execute (i.e. consider issues with BOMs / UTF-8 vs UTF-16 / etc). Passing files makes things simpler in that case.

While the PowerShell module takes the above approach, ultimately it seems inappropriate for Bolt given the use cases. Any performance issues with writing scripts to disk and sending them to remote hosts should be mitigated in later efforts that explore using SMB instead of the WinRM gem to send files.

This PR also fixes handling of exit status properly when run on PowerShell < 5. There is no ticket filed, and we're currently not testing on PS < 5, though I did put together a PR that was attempting to downgrade PowerShell to 4 in AppVeyor for running tests - https://github.com/puppetlabs/bolt/pull/162. So far, that attempt has been a failure, so it might be simplest to enable tests in Jenkins / vmpooler for downlevel PowerShell.

Comment by Michelle Fredette [ 2017/11/28 ]

added new feature release note.

Generated at Mon Dec 16 04:18:36 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.