Merge pull request #10347 from briancain/add-timeout-for-ssh-info-hyperv

Fixes #10229: Add proper reboot capability for Windows guests

plugins/communicators/winrm/communicator.rb

@@ -109,7 +109,7 @@ module VagrantPlugins
 
         @logger.info("WinRM is ready!")
         return true
-      rescue Errors::TransientError => e
+      rescue Errors::TransientError, VagrantPlugins::CommunicatorWinRM::Errors::WinRMNotReady => e
         # We catch a `TransientError` which would signal that something went
         # that might work if we wait and retry.
         @logger.info("WinRM not up: #{e.inspect}")

plugins/guests/windows/cap/change_host_name.rb

@@ -1,8 +1,9 @@
+require "log4r"
+
 module VagrantPlugins
   module GuestWindows
     module Cap
       module ChangeHostName
-
         def self.change_host_name(machine, name)
           change_host_name_and_wait(machine, name, machine.config.vm.graceful_halt_timeout)
         end
@@ -16,9 +17,6 @@ module VagrantPlugins
           script = <<-EOH
             $computer = Get-WmiObject -Class Win32_ComputerSystem
             $retval = $computer.rename("#{name}").returnvalue
-            if ($retval -eq 0) {
-              shutdown /r /t 5 /f /d p:4:1 /c "Vagrant Rename Computer"
-            }
             exit $retval
           EOH
 
@@ -27,13 +25,7 @@ module VagrantPlugins
             error_class: Errors::RenameComputerFailed,
             error_key: :rename_computer_failed)
 
-          # Don't continue until the machine has shutdown and rebooted
+          machine.guest.capability(:reboot)
-	  if machine.guest.capability?(:wait_for_reboot)
-            machine.guest.capability(:wait_for_reboot)
-          else
-            # use graceful_halt_timeout only if guest cannot wait for reboot
-            sleep(sleep_timeout)
-	  end
         end
       end
     end

plugins/guests/windows/cap/reboot.rb

@@ -1,21 +1,51 @@
+require "log4r"
+
 module VagrantPlugins
   module GuestWindows
     module Cap
       class Reboot
-        def self.wait_for_reboot(machine)
+        MAX_REBOOT_RETRY_DURATION = 120
-          # Technically it should be possible to make it work with SSH
+
-          # too, but we don't yet.
+        def self.reboot(machine)
-          return if machine.config.vm.communicator != :winrm
+          @logger = Log4r::Logger.new("vagrant::windows::reboot")
+          reboot_script = "shutdown /r /t 5 /f /d p:4:1 /c \"Vagrant Reboot Computer\""
+
+          comm = machine.communicate
 
           script  = File.expand_path("../../scripts/reboot_detect.ps1", __FILE__)
           script  = File.read(script)
-          while machine.communicate.execute(script, error_check: false) != 0
+          if comm.test(script, error_check: false, shell: :powershell)
+            @logger.debug("Issuing reboot command for guest")
+            comm.execute(reboot_script, shell: :powershell)
+          else
+            @logger.debug("A reboot is already in progress")
+          end
+
+          @logger.debug("Waiting for machine to finish rebooting")
+
+          wait_remaining = MAX_REBOOT_RETRY_DURATION
+          begin
+            wait_for_reboot(machine)
+          rescue Vagrant::Errors::MachineGuestNotReady, WinRM::WinRMHTTPTransportError => e
+            raise if wait_remaining < 0
+            @logger.warn("Machine not ready, cannot start reboot yet. Trying again")
+            sleep(5)
+            wait_remaining -= 5
+            retry
+          end
+        end
+
+        def self.wait_for_reboot(machine)
+          script  = File.expand_path("../../scripts/reboot_detect.ps1", __FILE__)
+          script  = File.read(script)
+
+          while machine.guest.ready? && machine.communicate.execute(script, error_check: false, shell: :powershell) != 0
             sleep 10
           end
 
           # This re-establishes our symbolic links if they were
           # created between now and a reboot
-          machine.communicate.execute("net use", error_check: false)
+          machine.communicate.execute("net use", error_check: false, shell: :powershell)
         end
       end
     end

plugins/guests/windows/plugin.rb

@@ -64,6 +64,11 @@ module VagrantPlugins
         Cap::Reboot
       end
 
+      guest_capability(:windows, :reboot) do
+        require_relative "cap/reboot"
+        Cap::Reboot
+      end
+
       guest_capability(:windows, :choose_addressable_ip_addr) do
         require_relative "cap/choose_addressable_ip_addr"
         Cap::ChooseAddressableIPAddr

plugins/providers/hyperv/provider.rb

@@ -38,6 +38,7 @@ module VagrantPlugins
         # This method will load in our driver, so we call it now to
         # initialize it.
         machine_id_changed
+        @logger = Log4r::Logger.new("vagrant::hyperv::provider")
       end
 
       def action(name)
@@ -83,16 +84,27 @@ module VagrantPlugins
         "Hyper-V (#{id})"
       end
 
+      # @return [Hash]
       def ssh_info
         # We can only SSH into a running machine
         return nil if state.id != :running
 
         # Read the IP of the machine using Hyper-V APIs
-        network = @driver.read_guest_ip
+        guest_ip = nil
-        return nil if !network["ip"]
+
+        begin
+          network_info = @driver.read_guest_ip
+          guest_ip = network_info["ip"]
+        rescue Errors::PowerShellError
+          @logger.warn("Failed to read guest IP.")
+        end
+
+        return nil if !guest_ip
+
+        @logger.debug("IP: #{guest_ip}")
 
         {
-          host: network["ip"],
+          host: guest_ip,
           port: 22,
         }
       end

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

@@ -68,6 +68,11 @@ describe VagrantPlugins::CommunicatorWinRM::Communicator do
       expect(subject.ready?).to be(false)
     end
 
+    it "returns false if hostname command fails with a WinRMNotReady error" do
+      expect(shell).to receive(:cmd).with("hostname").and_raise(VagrantPlugins::CommunicatorWinRM::Errors::WinRMNotReady)
+      expect(subject.ready?).to be(false)
+    end
+
     it "raises an error if hostname command fails with an unknown error" do
       expect(shell).to receive(:cmd).with("hostname").and_raise(Vagrant::Errors::VagrantError)
       expect { subject.ready? }.to raise_error(Vagrant::Errors::VagrantError)

test/unit/plugins/guests/windows/cap/reboot_test.rb

@@ -9,14 +9,33 @@ describe "VagrantPlugins::GuestWindows::Cap::Reboot" do
   let(:vm) { double("vm") }
   let(:config) { double("config") }
   let(:machine) { double("machine") }
+  let(:guest) { double("guest") }
   let(:communicator) { double("communicator") }
 
   before do
     allow(machine).to receive(:communicate).and_return(communicator)
+    allow(machine).to receive(:guest).and_return(guest)
+    allow(machine.guest).to receive(:ready?).and_return(true)
     allow(machine).to receive(:config).and_return(config)
     allow(config).to receive(:vm).and_return(vm)
   end
 
+  describe ".reboot" do
+    before do
+      allow(vm).to receive(:communicator).and_return(:winrm)
+    end
+
+    it "reboots the vm" do
+        allow(communicator).to receive(:execute)
+
+        expect(communicator).to receive(:test).with(/# Function/, { error_check: false, shell: :powershell }).and_return(0)
+        expect(communicator).to receive(:execute).with(/shutdown/, { shell: :powershell }).and_return(0)
+        expect(described_class).to receive(:wait_for_reboot)
+
+        described_class.reboot(machine)
+    end
+  end
+
   describe "winrm communicator" do
     before do
       allow(vm).to receive(:communicator).and_return(:winrm)
@@ -24,7 +43,7 @@ describe "VagrantPlugins::GuestWindows::Cap::Reboot" do
 
     describe ".wait_for_reboot" do
       it "runs reboot detect script" do
-        expect(communicator).to receive(:execute).with(/# Function/, { error_check: false }).and_return(0)
+        expect(communicator).to receive(:execute).with(/# Function/, { error_check: false, shell: :powershell }).and_return(0)
         allow(communicator).to receive(:execute)
 
         described_class.wait_for_reboot(machine)
@@ -32,7 +51,7 @@ describe "VagrantPlugins::GuestWindows::Cap::Reboot" do
 
       it "fixes symlinks to network shares" do
         allow(communicator).to receive(:execute).and_return(0)
-        expect(communicator).to receive(:execute).with('net use', { error_check: false })
+        expect(communicator).to receive(:execute).with('net use', { error_check: false, shell: :powershell })
 
         described_class.wait_for_reboot(machine)
       end
@@ -45,8 +64,9 @@ describe "VagrantPlugins::GuestWindows::Cap::Reboot" do
     end
 
     describe ".wait_for_reboot" do
-      it "does not execute Windows reboot detect script" do
+      it "does execute Windows reboot detect script" do
-        expect(communicator).to_not receive(:execute)
+        expect(communicator).to receive(:execute).with(/# Function/, { error_check: false, shell: :powershell }).and_return(0)
+        expect(communicator).to receive(:execute).with('net use', { error_check: false, shell: :powershell })
         described_class.wait_for_reboot(machine)
       end
     end

test/unit/plugins/providers/hyperv/provider_test.rb

@@ -3,7 +3,12 @@ require_relative "../../../base"
 require Vagrant.source_root.join("plugins/providers/hyperv/provider")
 
 describe VagrantPlugins::HyperV::Provider do
-  let(:machine) { double("machine") }
+  let(:driver){ double("driver") }
+  let(:provider){ double("provider", driver: driver) }
+  let(:provider_config){ double("provider_config", ip_address_timeout: ip_address_timeout) }
+  let(:ip_address_timeout){ 1 }
+  let(:machine){ double("machine", provider: provider, provider_config: provider_config) }
+
   let(:platform)   { double("platform") }
   let(:powershell) { double("powershell") }
 
@@ -103,4 +108,28 @@ describe VagrantPlugins::HyperV::Provider do
       expect(subject.state.id).to eq(:bar)
     end
   end
+
+  describe "#ssh_info" do
+    let(:result) { "127.0.0.1" }
+    let(:exit_code) { 0 }
+    let(:ssh_info) {{:host=>result,:port=>22}}
+
+    before do
+      allow(VagrantPlugins::HyperV::Driver).to receive(:new).and_return(driver)
+      allow(machine).to receive(:action).with(:read_state).and_return(machine_state_id: :running)
+    end
+
+    it "returns nil if a PowerShellError is returned from the driver" do
+      allow(driver).to receive(:read_guest_ip)
+        .and_raise(VagrantPlugins::HyperV::Errors::PowerShellError, script: anything, stderr: anything)
+      expect(subject.ssh_info).to eq(nil)
+    end
+
+    it "should receive a valid address" do
+      allow(driver).to receive(:execute).with(:get_network_config).and_return(result)
+
+      allow(driver).to receive(:read_guest_ip).and_return({"ip" => "127.0.0.1"})
+      expect(subject.ssh_info).to eq(ssh_info)
+    end
+  end
 end