Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smb synced folders for Windows "guests" #47

Merged
merged 9 commits into from
May 22, 2015

Conversation

chrisbaldauf
Copy link
Contributor

Change default synced_folders behavior on Windows host/guest pair to use SMB file sharing, rather than the very slow uploading over WinRM.

I've tested this on a Win7 host and Windows Server 2008 managed host. You will be prompted for credentials if you don't include them in the Vagrantfile per the docs

FWIW, I'm planning on adding functionality to Vagrant Orchestrate that will prompt you once for credentials and pass them to winrm and SMB.

I assumed that this would be a big enough change to warrant a 0.7 release, but if that isn't the case, let me know and I can turn this into a 0.6.x change.

Meant to address #46

Happy to discuss if you have any feedback.

@chrisbaldauf chrisbaldauf changed the title Smb synced folders Smb synced folders for Windows "guests" May 14, 2015
@@ -26,21 +26,6 @@ def self.action_up

b2.use LinkServer
end
=begin
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just removing commented out code for cleanliness.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :-) 👍

…use SMB file sharing, rather than the very slow uploading over WinRM.

no h in sync

Revent ip change used to work around mac vbox routing table issue
@tknerr
Copy link
Owner

tknerr commented May 19, 2015

Please help me understand:

From what I have experienced playing around with automating windows guests, I got the impression that vagrant by default uses WinRM for synced folders too? I'm not 100% sure with that, but it was my impression at least (and I never ran it from an admin shell and I was never prompted for passwords either).

Is it right, that the Vagrant::Action::Builtin::SyncedFolders determine that automatically? My assumption is that it would use winrm by default, but also allow you to switch to the smb implementation as per the docs you linked above.

I might be totally wrong, just trying to understand.

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

Trying to get an example running, and I see what you mean now with winrm uploads being "painfully slow" ;-)

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

However, I have some kind of weird behaviour:

  • using the state on master, my example works (using winrm upload, works but is really slow)
  • using your PR, it uses the vagrant builtin syncedfolder impl but then tries to upload via rsync?! (which fails of course)

May that be because I am not running from a privileged shell?

My example and debug gist follows...

@chrisbaldauf
Copy link
Contributor Author

Were you running this from a Windows or non-Windows host? I also didn't see the output gist, my apologies if I missed something obvious.

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

I'm running this from a windows host, using the latest tknerr/bills-kitchen version. It has rsync.exe in the PATH, maybe that makes a difference

Didn't come to uploading the gist yet, was interrupted by $dayjob ;-)

@chrisbaldauf
Copy link
Contributor Author

I'm trying to repro using the Vagrantfile in your WIP branch on my Windows host and will report back what I find.

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

So, here's my current state:

  • 6ecae54 on the windows-examples branch contains the current state as is (no smb support yet), with the added windows example. It works, uses WinRM and is slooooow
  • 78aca82 on the windows-examples branch merges in your smb changes. It doesn't work in my environmnet (win7 / bills-kitchen) as it tries to use rsync. The gist with debug log here.

Haven't had the chance to analyze the log output yet, and will be afk for a while now.

Most interesting for me would be if you can reproduce it, or whether it's specific to my env...

@chrisbaldauf
Copy link
Contributor Author

I'll check the gist and see if it differs from what I'm seeing. I was using
a "real" managed server in my testing, so I'll try to control for that as
well. Thanks for taking a look!

On Tue, May 19, 2015 at 10:48 AM, Torben Knerr notifications@github.com
wrote:

So, here's my current state:

  • 6ecae54
    6ecae54
    on the windows-examples branch contains the current state as is (no smb
    support yet), with the added windows example. It works, uses WinRM and is
    slooooow
  • 78aca82
    78aca82
    on the windows-examples branch merges in your smb changes. It doesn't work
    in my environmnet (win7 / bills-kitchen) as it tries to use rsync. The gist
    with debug log https://gist.github.com/tknerr/c4b891f2b59ef1a52019
    here.

Haven't had the chance to analyze the log output yet, and will be afk for
a while now.

Most interesting for me would be if you can reproduce it, or whether it's
specific to my env...


Reply to this email directly or view it on GitHub
#47 (comment)
.

@chrisbaldauf
Copy link
Contributor Author

Update: while trying to debug another, semi-unrelated issue, I was able to repro your case. I'm digging in further now.

@chrisbaldauf
Copy link
Contributor Author

I think I've figured out what was going on - the SMB synced folder action was not usable? from your machine because either a) you weren't running from an admin command prompt or b) you don't have PowerShell 3 or greater installed. I've revised the implementation to check whether the SMB folder sync is usable and if not - 1) print why and 2) use WinRM. Please consider the following Proof of Concept code

      def self.action_provision
        Vagrant::Action::Builder.new.tap do |b|
          b.use ConfigValidate
          b.use WarnNetworks
          b.use Call, IsLinked do |env, b2|
            unless env[:result]
              b2.use MessageNotLinked
              next
            end

            b2.use Call, IsReachable do |env, b3|
              unless env[:result]
                b3.use MessageNotReachable
                next
              end

             b3.use Provision
              if env[:machine].config.vm.communicator == :winrm
                env[:ui].info("Checking if SMB folder sync is usable")
                smb_usable = false
                begin
                  smb_synced_folder_class = Vagrant.plugin("2").manager.synced_folders[:smb][0]
                  smb_usable = smb_synced_folder_class.new.usable?(machine: nil, raise_error: true)
                rescue => ex
                  env[:ui].warn("Unable to use SMB folder sync, falling back to WinRM")
                  env[:ui].warn(ex.message)
                end

                if smb_usable
                  b3.use Vagrant::Action::Builtin::SyncedFolders
                else
                  b3.use SyncFolders
                end
              else
                # Vagrant managed servers custom implementation
                b3.use SyncFolders
              end
            end
          end
        end
      end

I still need to handle a few other things, but it would be good to validate this new approach works for you before I invest more time.

TODO:

  • Cleanup synced folders (not good to leave SMB symlinks mounted from prod to your desktop)
  • Restore the WinRM code in action/sync_folders.rb
  • See if there is a better place for this code to live, it seems rather out of place.

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

@chrisbaldauf let me do some further investigation. I just noted that the initial gist I posted was completely useless, as the remote windows server was not running. Just updated it with the more meaningful log output. Can not see from the logs why it thinks that rsync would be the best option though....

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

After removing rsync.exe from the PATH I now get Vagrant::Errors::NoDefaultSyncedFolderImpl

...
 INFO interface: error: Vagrant::Errors::NoDefaultSyncedFolderImpl: No synced folder implementation is available for your synced folders!
Please consult the documentation to learn why this may be the case.
You may force a synced folder implementation by specifying a "type:"
option for the synced folders. Available synced folder implementations
are listed below.

docker, nfs, rsync, smb, virtualbox
Vagrant::Errors::NoDefaultSyncedFolderImpl: No synced folder implementation is available for your synced folders!
Please consult the documentation to learn why this may be the case.
You may force a synced folder implementation by specifying a "type:"
option for the synced folders. Available synced folder implementations
are listed below.

Also updated the gist with debug log output from the without-rsync case

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

After telling vagrant explicitly to use "smb":

    ms_config.vm.synced_folder ".", "/vagrant", type: "smb"

I now get a clear error message:

...
VagrantPlugins::SyncedFolderSMB::Errors::WindowsAdminRequired: SMB shared folders require running Vagrant with administrative
privileges. This is a limitation of Windows, since creating new
network shares requires admin privileges. Please try again in a
console with proper permissions or use another synced folder type.

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

Once I run the above from an elevated command prompt to make smb work, vagrant seems to hang after checking the powershell version. Also looks like I have powershell v2 installed, so I would need v3 anyway...

Here's where it stalls:

...
 INFO provision: Checking provisioner sentinel file...
 INFO warden: Calling IN action: #<Berkshelf::Vagrant::Action::Install:0x5e2fd68>
 INFO interface: warn: Berkshelf plugin is disabled but a Berksfile was found at your configured path: C:/Repos/_github/vagrant-managed-servers/Berksfile Berkshelf plugin is disabled but a Berksfile was found at your configured path: C:/Repos/_github/vagrant-managed-servers/Berksfile
 INFO interface: warn: Enable the Berkshelf plugin by setting 'config.berkshelf.enabled = true' in your vagrant config
Enable the Berkshelf plugin by setting 'config.berkshelf.enabled = true' in your vagrant config
 INFO warden: Calling IN action: #<Berkshelf::Vagrant::Action::Upload:0x5e2fd20>
 INFO warden: Calling IN action: #<VagrantPlugins::Omnibus::Action::InstallChef:0x5e2fd08>
 INFO warden: Calling IN action: #<Vagrant::Action::Builtin::SyncedFolders:0x5e14c18>
 INFO subprocess: Starting process: ["C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\/powershell.EXE", "-NoProfile", "-ExecutionPolicy", "Bypass", "$PSVersionTable.PSVersion.Major"]
DEBUG subprocess: Selecting on IO
DEBUG subprocess: stdout: 2

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

Also, once I am in an elevated shell, I no longer need this line in the managed server config:

 ms_config.vm.synced_folder ".", "/vagrant", type: "smb"

Even without the config above, it will try to use smb when run from an elevated shell. Still it stalls at the same place as mentioned above

@chrisbaldauf
Copy link
Contributor Author

I was able to reproduce on a windows machine running powershell 2.0. I think I've got enough feedback for now and I'll follow up when I've got something ready. Thanks!

On May 19, 2015, at 5:53 PM, Torben Knerr notifications@github.com wrote:

Once I run the above from an elevated command prompt to make smb work, vagrant seems to hang after checking the powershell version. Also looks like I have powershell v2 installed, so I would need v3 anyway...

Here's where it stalls:

...
INFO provision: Checking provisioner sentinel file...
INFO warden: Calling IN action: #Berkshelf::Vagrant::Action::Install:0x5e2fd68
INFO interface: warn: Berkshelf plugin is disabled but a Berksfile was found at your configured path: C:/Repos/_github/vagrant-managed-servers/Berksfile Berkshelf plugin is disabled but a Berksfile was found at your configured path: C:/Repos/_github/vagrant-managed-servers/Berksfile
INFO interface: warn: Enable the Berkshelf plugin by setting 'config.berkshelf.enabled = true' in your vagrant config
Enable the Berkshelf plugin by setting 'config.berkshelf.enabled = true' in your vagrant config
INFO warden: Calling IN action: #Berkshelf::Vagrant::Action::Upload:0x5e2fd20
INFO warden: Calling IN action: #VagrantPlugins::Omnibus::Action::InstallChef:0x5e2fd08
INFO warden: Calling IN action: #Vagrant::Action::Builtin::SyncedFolders:0x5e14c18
INFO subprocess: Starting process: ["C:\Windows\System32\WindowsPowerShell\v1.0/powershell.EXE", "-NoProfile", "-ExecutionPolicy", "Bypass", "$PSVersionTable.PSVersion.Major"]
DEBUG subprocess: Selecting on IO
DEBUG subprocess: stdout: 2

Reply to this email directly or view it on GitHub.

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

@chrisbaldauf thanks, too!

I think we'll need both: the existing slow (but most compatible) winrm upload, and if usable? the better smb support for hosts that support it.

We can try to autodetect what's better, but it would be also nice to have config option to override.

I would also be 100% fine with:

  • by default always use winrm upload
  • only if smb is explicitly configured (via the standard config, e.g. config.vm.synced_folder ".", "/vagrant", type: "smb") use smb

That way we would not need any auto-detection at all (but also have no auto-detection ;-)) and would not need to introduce an additional config option.

If you say that auto-detection is useful for you, let's add it. You care more about the windows / smb support than I do currently. And you contributed the whole windows support. So the decision should definitely be up to you

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

For the sake of documentation and completeness:

Without an elevated prompt: not usable? in my environment

C:\Repos\_github\vagrant-managed-servers>bundle exec irb
Your Gemfile lists the gem vagrant-managed-servers (>= 0) more than once.
You should probably keep only one of them.
While it's not a problem now, it could cause errors if you change the version of just one of them later.
irb(main):001:0> require 'vagrant'
=> true
irb(main):002:0> x = Vagrant.plugin("2").manager.synced_folders[:smb][0]
=> VagrantPlugins::SyncedFolderSMB::SyncedFolder
irb(main):003:0> x.new.usable?(machine: nil, raise_error: true)
=> false

Within an elevated prompt: aksing if usable? is stalling and never returns

C:\Repos\_github\vagrant-managed-servers>bundle exec irb
Your Gemfile lists the gem vagrant-managed-servers (>= 0) more than once.
You should probably keep only one of them.
While it's not a problem now, it could cause errors if you change the version of just one of them later.
irb(main):001:0> require 'vagrant'
=> true
irb(main):002:0>  x = Vagrant.plugin("2").manager.synced_folders[:smb][0]
=> VagrantPlugins::SyncedFolderSMB::SyncedFolder
irb(main):003:0> x.new.usable?(machine: nil, raise_error: true)

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

@chrisbaldauf if we wanted to do it really clean and in line with the vagrant philosophy, this would probably mean:

  • always use Vagrant::Action::Builtin::SyncedFolders, but...
  • ...refactor and register the winrm upload as a synced_folder plugin (akin to the existing ones)
  • ...rely on the existing rsync plugin instead of having our own rsync synced folder implementation

This would be the best / cleanest solution imho, but probably the most effort too. I don't have time to invest for this right now, though :-/. If you have, feel free to go ahead, I'll happily support you with testing. If not, let's make it work first and then make it nice (later, in a second step).

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

Thinking again... maybe the above clean approach would not be so much extra work though, guess it could save us some work afterwards, e.g.:

  • we could just say config.vm.synced_folder ".", "/vagrant", type: "winrm" to force winrm (no additional config option required)
  • less code to maintain here, fixing bugs centrally in vagrant core (but: less flexibility)
  • handle mixed type multiple synced folders out of the box (never really tried that, and probably doesn't make that much sense either...)

@tknerr
Copy link
Owner

tknerr commented May 19, 2015

On reusing the existing rsync plugin:

Just tested: works fine for me on my win7 host, yet I still need to work around hashicorp/vagrant#4073 by appending the "/cygpath" prefix here (which I needed to do anyway if I ever wanted to use rsync shared folders from my win7 host...)

@chrisbaldauf
Copy link
Contributor Author

The problem that I have with setting the type: "smb" for synced folders is some provisioners (Puppet in particular) create synced folders dynamically. Adding the WinRM folder sync to Vagrant core feels wrong to me, since the only reason that I implemented that in the first place was really as a test to see if this was a valid approach on Windows - it turns out it is. From where I stand, I think the best balance is to let the default remain as is - WinRM folder synching with the managed-server specific sync_folders action. Add a configuration option that allows you to force override synced_folders type for all managed servers and allow the user to set that to smb if they like. If that is set, we try to use SMB and error otherwise.

I do like the idea of using the built in SyncFolders action, but then we lose the WinRM upload, which seems to be fast enough and pretty reliable for small numbers of files / folders. Do you think a WinRM synced_folder type would have general value to Windows users? Do you think they'd choose it over SMB? Do you have any idea on the turnaround time for submitting features to Vagrant core?

@tknerr
Copy link
Owner

tknerr commented May 20, 2015

Hey @chrisbaldauf, I didn't meant to contribute the winrm synced folder to vagrant core. I rather meant to construct it ike the other synced folder plugin implementations, i.e.:

  • extend Vagrant.plugin("2", :synced_folder) and implement the prepare, enable, usable? and cleanup methods like for smb here or for rsync here
  • register it with a name (e.g. "winrm") and priority using the plugin api (e.g. like smb here or rsync here)

I see the problem with plugins creating synced folders dynamically. And I believe all these plugins (same for Chef I guess) rely on Vagrant to decide which of the available synced folder implementations is the best (if we are using Vagrant::Action::Builtin::SyncedFolders at least).

One way to fix it would be to register the "winrm" synced folder implementation with a priority of 6:

  • the smb synced folder impl as a priority of 7
  • and rsync has a priority of 5

Putting the winrm plugin inbetween would mean it would try smb.usable? first, and if not winrm.usable?. That would pretty much reflect what we want (use smb if usable, otherwise fall back to winrm before trying rsync) as a generalized default behaviour for everyone who uses Vagrant::Action::Builtin::SyncedFolders. What do you think?

Btw: I didn't mean to contribute the winrm synced folder to vagrant core, but instead just use the synced folder plugin API as (I believe is) intended. It could still live here in this plugin, but since it's a generally useful addition, it would also justify for a dedicated "vagrant-winrm-syncedfolder" plugin.

While my current feeling says this is the right way to do it, I would still be okay with keeping our own SyncFolders action for now and adding a config option for forcing smb, so that we can quickly release something that fulfills your need without doing the extra effort now. I would create a separate issue for this refactoring then.

@chrisbaldauf
Copy link
Contributor Author

Ah, I see - not putting WinRM synced folders into the core makes it a much easier sell. I think your proposal is the right way to go and I'll work on pulling something together. I had noticed that there was room to squeeze the WinRM with a priority of 6 in there, but I incorrectly figured it should go into Vagrant core.

I think the biggest blocker at this point is the hanging Powershell version check when run on Powershell 2.0. I'm able to reproduce that regularly and I believe we need an answer to that before any of this works, since if you're running PowerShell 2.0, it won't give you an error message about upgrading, it will just hang forever.

@tknerr
Copy link
Owner

tknerr commented May 20, 2015

Ah right, the powershell 2.0 issue is indeed annoying. I usually don't run
into it because I'm never running inside a privileged shell usually...

Would be much better if vagrant would just elevate for that specific
purpose rather than expecting you to run from an elevated shell up front.
Am 20.05.2015 14:57 schrieb "Chris Baldauf" notifications@github.com:

Ah, I see - not putting WinRM synced folders into the core makes it a much
easier sell. I think your proposal is the right way to go and I'll work on
pulling something together. I had noticed that there was room to squeeze
the WinRM with a priority of 6 in there, but I incorrectly figured it
should go into Vagrant core.

I think the biggest blocker at this point is the hanging Powershell
version check when run on Powershell 2.0. I'm able to reproduce that
regularly and I believe we need an answer to that before any of this works,
since if you're running PowerShell 2.0, it won't give you an error message
about upgrading, it will just hang forever.


Reply to this email directly or view it on GitHub
#47 (comment)
.

@chrisbaldauf
Copy link
Contributor Author

Just pushed something that I believe should work (save the powershell version check hanging bug). You can test that the new WinRM synced folder plugin works by patching the SMB usable to return false immediately here.

My intention is to move the winrm folder sync to another repo and publish it as its own plugin, just wanted to check that it worked integrated first. There is still more work to do in cleaning up SMB folders, but it certainly feels cleaner and as if we're making progress.

I left the non-winrm implementation as is, since it seems like there is real risk that the behavior could differ between builtin Rsync synced folders and the VMS implementation. If you feel that this is something that is required, let me know.

mechanism, most notably that file tranfer is slow for large numbers of files.
EOF

synced_folder("wirnm", 6) do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "wirnm" instead of "winrm" ;-)

@tknerr
Copy link
Owner

tknerr commented May 20, 2015

The WinRM synced folder plugin looks good to me, and works as advertised:

  • by default it chooses now WinRM on my machine (since I'm not running an elevated shell)
  • I can take control over the synced folder mechanism like this now:
    • mw_config.vm.synced_folder ".", "/vagrant", type: "smb" => fails as expexted due to non-elevated shell
    • mw_config.vm.synced_folder ".", "/vagrant", type: "rsync" => fails as expected
    • mw_config.vm.synced_folder ".", "/vagrant", type: "winrm" => works as expected, too! :-)

👍 for publishing it as a separate plugin, so that one can benefit from it independently of the VMS plugin

As for the VMS specific rsync implementation: yes, let's keep it in for now until we have a clean workaround for hashicorp/vagrant#4073 (which the VMS implementation does not suffer from). My idea was to publish this via a "vagrant-rsync-on-windows-monkey-patch" plugin (probably need to find a better name ;-)) first

@tknerr
Copy link
Owner

tknerr commented May 20, 2015

To summarize

When provisioning managed Windows servers (more precisely: with config.vm.communicator == :winrm):

  • from Windows hosts (when running from an elevated shell and with PowerShell 3.0 installed) it should now use SMB
  • from Mac / Linux or Windows hosts with non-elevated shells it should fall back to WinRM

There are still open bugs with SMB, but these are actually vagrant core smb plugin bugs:

  • it stalls when powershell 2.0 instead of 3.0 is installed
  • SMB synced folders are not cleaned up

I'm fine with releasing the current state, it's not perfect but definitely an improvement.

Remaining question about the externalizing the winrm plugin: once we have this as a separate plugin, can we depend on it, so that it always gets installed along with VMS if not present? Are inter-plugin dependencies in Vagrant a good idea or bad idea?

@chrisbaldauf
Copy link
Contributor Author

I've made solid progress in creating the vagrant-winrm-syncedfolders plugin and should have a candidate release today. I agree with your summary above and was just about to ask the question to make sure that you were ok with making the syncedfolders plugin a dependency of vagrant-managed-servers. I found at least one example of inter-vagrant-plugin dependencies, so I believe that it should work without issue, since vagrant plugins are just gems. I think adding a dependency is the right way to go and I'll test it out to make sure it works.

@chrisbaldauf
Copy link
Contributor Author

I've pushed something that I think lines up with your summary above. Please review and let me know if there is anything else that you think I can do. Thanks so much for all of your help and feedback! One user that is using this for an internal project said that (the patched SMB support that I gave him) took the provisioning time on Windows from 20 minutes to 90 seconds.

👍

@tknerr
Copy link
Owner

tknerr commented May 21, 2015

That is awesome 👍 👍 👍. 20 mins to 90 secs deserves a triple-thumbs-up at mininum! He should definitely buy you some 🍻 for that :-)

Thanks for doing all the work and making vagrant a happier place for windows users!

I will be giving it a quick test again and probably push out a new VMS release tomorrow.

@chrisbaldauf
Copy link
Contributor Author

Thanks. Before that, I'll add some more info to the readme on the Windows
folder sync.

On Thu, May 21, 2015 at 5:43 PM, Torben Knerr notifications@github.com
wrote:

That is awesome [image: 👍] [image: 👍] [image: 👍]. 20 mins to 90
secs deserves a triple-thumbs-up at mininum! He should definitely buy you
some [image: 🍻] for that :-)

Thanks for doing all the work and making vagrant a happier place for
windows users!

I will be giving it a quick test again and probably push out a new VMS
release tomorrow.


Reply to this email directly or view it on GitHub
#47 (comment)
.

@tknerr
Copy link
Owner

tknerr commented May 21, 2015

perfect 👍, will hold on for that

@chrisbaldauf
Copy link
Contributor Author

Readme updated.

tknerr added a commit that referenced this pull request May 22, 2015
Smb synced folders for Windows "guests"
@tknerr tknerr merged commit 187152d into tknerr:master May 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants