WinRb / winrm-elevated

Runs PowerShell commands as elevated over Windows Remote Management (WinRM) via a scheduled task
Apache License 2.0
27 stars 17 forks source link

Use executor, latest winrm-fs and add appveyor #10

Closed mwrock closed 8 years ago

mwrock commented 8 years ago

I'm looking to add this gem to Test-kitchen and provide the option for test-kitchen runs to be elevated. Needed to bump winrm-fs here to prevent gem conflicts and also getting rid of the deprecation warnings.

While we are at it, I pulled in travis and appveyor integration.

mwrock commented 8 years ago

I'll release once @sneal is cool with these changes.

mwrock commented 8 years ago

Marking this WIP as I'm now running into issues with Test-Kitchen and Mixlib-Install's installation of the chef MSI. They basically use Start-Process -wait ... to kick off msiexec.exe but then this gem dies trying to delete the out/err files because another process is using them. I need to troubleshoot later but I'm guessing we may need to d omore than wait on the tasks registered state. I remember running into something like this with boxstarter and I ended up waiting on the powershell process that the scheduled task calls. Not certain we need to do that but (maybe we just need a small delay) needs investigation.

mwrock commented 8 years ago

Ends up the above mentioned task was returning exit code 259 indicating that a thread is STILL_ACTIVE. I refactored the powershell to find the powershell process run by the task and then wait on that task.

This brings up an unfortunate challenge in guaranteeing a correct exit code. In this case, the task's exit code remains 259 even after the powershell completes. Further, grabbing the process via Get-Process always returns a $NULL exit code.

We could probably do something with wrapping the script in try/catch, getting its $LastExitCode and then sending that somewhere. That will be left for another day/release.

mwrock commented 8 years ago

Removed WIP. Passes integration tests here as well as working with full kitchen runs. I can install a .net framework with no complaints!

mwrock commented 8 years ago

I added the ability to run the task as a service account (ie System). This is useful for Test-Kitchen scenarios where a node will run the Chef service as System and you want Test-Kitchen to simulate running under that user context.

sneal commented 8 years ago

Lots of good things here. The installer exit code... janky. I guess I want to know why when running an installer via a scheduled task we're sometimes seeing exit code 259? It makes me think there's another upstream issue.

mwrock commented 8 years ago

Yeah I'm really not sure. Googling turned up various SO links that did not go deep into the issue and just basically said "this happens sometimes" and there were also a couple packer issues around its winrm based provisioner complaining about this.

The particular script where I see this happen consistently is a long script where Test-Kitchen checks to see if a certain version of chef is installed, downloads it if needed and then installs it. This only happens if it has to run MSIEXEC. I'm guessing it might have something to do with the fact of the task is spawning another process. The sceipt uses Start-Process -wait .... I tried changing it to just use straightup .NET Process and ProcessStartInfo but that had no effect.

I'm guessing this is a reality we cant avoid but I'll look to see if I cant find some more definitive reasoning behind it.

mwrock commented 8 years ago

ok I discovered whats up with the 259 Exit code. It has nothing to do with MSIEXEC or spawned processes specifically. It is directly related to the task timeout. By default the task is given the timeout of the underlying WinRM service and that defaults to a minute. When the task times out it does not kill any child processes, it just ends and reports back 259 that the task process is still running.

By hacking the timeout to 2 hours, I get a 0 exit code. So its just that the MSIEXEC here takes a long time which causes this.

I think we may want to give callers the ability to set a separate timeout. While one can certainly reconfigure the WinRM timeout, there are two issues I see here:

  1. Its not clear at all that the task timeout comes from the WinRM OperationTimeout
  2. The WinRM OperationTimeout is IMO a bogus timeout that has no real effects the way that the task timeout has. That timout simply sends a fault code to the WinrmService which responds by reissuing a new ReceiveRequest.

I propose we add a new argument to powershell_elevated that defaults to the service timeout. Ideally I think it would default to something really long but I'm concerned we may break backwards compatibility by setting it to something different.

I do this here and add some words to the readme. @sneal let me know if you think there is a better approach, but it does look like we can jetison the WMI process querying (yay).

mwrock commented 8 years ago

Thinking about this more @sneal perhaps we should just hard code the timeout to 24 hours. Currently the timeout is nothing but disruptive because the underlying process is locking the stdout/stderr files and a timeout is therefore not graceful but manifests itself as an error when trying to delete those files.

We could catch that and just leave the files behind if this happens and also intelligently detect a 259 exit code and print a warning that the task timed out but I'm not sure i see the value of a timeout here either for the vagrant or test-kitchen use cases. If the user uses a "non elevated" session, there is no concept of a timeout so why introduce it only for the elevated case?

Since this gem is young and pre 1.0, I don't see much harm here in this approach from a backwards compatibility point of view.

Thoughts?

sneal commented 8 years ago

Nice job figuring out the real issue is the task timeout. I'm all for changing the task timeout to something artificially long. If it ever comes up in the future (seems doubtful), we can always add a configurable task timeout.

mwrock commented 8 years ago

ok I think this is good for a review @sneal

mwrock commented 8 years ago

ok. Just pushed up change to initializer to take an executor @sneal

sneal commented 8 years ago

I like it! :shipit: