fix #7489 preventing winrm connection leakage #7712
Conversation
|
I'm happy to confirm that this resolved all of my winrm issues. I tested this on two different Windows 7 images, Windows 2008r2 Enterprise, two different Windows 2012r2 Standard images, and Windows 10. All worked correctly. |
|
Hi Guys, e.g. config.vm.provision :shell, path: "scripts/provision/install-dot-net.ps1" the config.vm.provision :shell, path: "scripts/provision/configure-sql-port.ps1" will not run. Thanks for looking! |
|
@istvano I am trying to reproduce this and have been unsuccessful. Can you share what version of windows you are provisioning and a gist of the full error message preferably with Thanks |
|
Hi, Host: Windows 10
I have 10 provisioning scripts and when the second provisioning bat file runs it contains the content from the earlier file hence I believe it does not get overridden. I used this as the base for my project. I hope this helps. Kind Regards |
|
ok i was trying on 2012r2. I get a repro on 2008r2 and will look closer later today. |
|
Hi, Great. just try to use that Vagrant image I linked. it uses 2008 r2 and on my windows 10 it dies fail and can be reproduced everytime. Kind Regards |
|
Actually even simple scripts that have |
|
hi, I see. Ok so you can reproduce it which is the main thing. I used a work around to solve the problem in the meantime. I am using upload_path and different name for each script so I can work with it. Although I believe this "bug" is a side-effect of the patch and it should be looked at. Thanks again! |
|
So it looks like this change in particular is not causing the behavior that @istvano is seeing but "uncovering" it. If I revert the code, I get the original error related to too many shells but then if I limit the provisioner scripts to just 2 in order to keep the total count under 5, then is see the second provisioner calling the same script as the first. So while this PR "improves" things, there is indeed another problem at play here with windows 2008R2 and provisioning scripts which I will investigate further and address in a separate PR if it is truly separate. |
|
OK I see the issue now. It unfortunately is tied to a bug in winrm-fs where checking if a previously uploaded file already exists on the remote. The checksum comparison for 2008R2 is broken. This has been fixed in winrm V2 WinRb/WinRM#191 which has not yet been released. Its just waiting final code review now and once released I'll begin working to retrofit vagrant integration to use it. So I think this PR still stands to be merged and thanks a bunch @istvano for taking the time to test it and report this problem! |
|
You are welcome! Great work on discovering the other bit! |
|
@chrisroberts -- Is there an ETA for this to be integrated? I'm a little stuck in that I need some fixes for Chef that were delivered about 1.8.1, so reverting isn't an option. Just looking for a ballpark, thanks! |
|
We released in winrm-fs this last week. So we are gradually plugging it into consuming apps. I plan to retrofit vagrant with the winrm v2 (and family) gems in a few weeks after it has had a chance to "bake". |
|
@mwrock - Thanks for the quick response! I'll ensure that my team uses 1.8.1 for the time being. Don't want to mix topics, any ideas where I could ask some Chef/Vagrant/Windows questions? |
|
I think the best place is https://discourse.chef.io/ |
|
@mwrock - Thanks again my friend! |
|
@mwrock Just curious if you had a timeframe in mind for implementing this since it's been a few weeks? |
|
we have already addressed a couple bugs (some important I think and one was a major leakage hole). Later this week these gems will hit a broader audience when we release the chef-dk. I'm planning on giving that at least a few weeks. The main issue is that I have total control over those release pipelines whereas once its in vagrant its locked down for a while. So I'm exercising extra caution here. |
|
Sounds good. I've made these changes to my local environment and it fixed the problem, so I'm good with that until a broader release. |
|
I'm not sure if I should create a new issue for this or not, so sorry if this is the wrong choice. I'm still seeing the WinRM connection errors on 1.8.6. I'm specifically seeing it on the number of concurrent operations, not the number of shells. I was previously using 1.8.5 while monkey-patching in the fixes suggested here and had no issues. I did both an upgrade to 1.8.6 and a fresh install, but same error each time. One thing I noticed is that 1.8.6 for OSX still includes WinRM gem 1.8.1, but 2.0.1 is available. Would upgrading the WinRM gem fix the issue? |
|
I think this was merged right after the 1.8.6 release so you shouldn't expect to see it until the next release. |
|
Oh yup, I see that now. Sorry about that. I'll monkey patch 1.8.6 and hold out for the next release. Thanks! |
|
FYI, I just reported the problem with wrong execution the first script also for the subsequent ones in GH-7971. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
This ensures that all winrm commands open a new connection and close it after the command. While the previous shell code is "mostly" leak proof, it keeps a connection open for each
WinRMShellinstance and does not close them until the vagrant process exits. Thats generally a more efficient approach and reduces connection startup overhead for multiple commands. In the case of Windows 2008 R2, the default maximum open shells is 5. So if one uses multiple vagrant plugins, it may be easy to exceed this maximum and errors will ensue.The cost savings of openning new connections really does not outweigh the error scenarios possible here on Windows 2008R2 (and Windows 7).