From 54c8ebc31a287849b2813ed7337b633171f08d89 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Fri, 26 Oct 2018 16:39:59 -0700 Subject: [PATCH] Fixes #10229: Add timeout for changing hostname on windows Prior to this commit, if Windows was slow to reboot, Vagrant would fail to find the right IP address to upload the wait_for_reboot script to. This commit fixes this race condition by adding a timeout to ensure that Vagrant can retry. It also properly catches an exception in the winrm ready? method for checking if a guest is properly ready for communications. --- plugins/communicators/winrm/communicator.rb | 2 +- .../guests/windows/cap/change_host_name.rb | 31 ++++++++++++++----- plugins/providers/hyperv/provider.rb | 18 +++++++++-- .../plugins/providers/hyperv/provider_test.rb | 31 ++++++++++++++++++- 4 files changed, 69 insertions(+), 13 deletions(-) diff --git a/plugins/communicators/winrm/communicator.rb b/plugins/communicators/winrm/communicator.rb index 194eb1d8a..14a742f5b 100644 --- a/plugins/communicators/winrm/communicator.rb +++ b/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}") diff --git a/plugins/guests/windows/cap/change_host_name.rb b/plugins/guests/windows/cap/change_host_name.rb index 979627dc0..8526247f3 100644 --- a/plugins/guests/windows/cap/change_host_name.rb +++ b/plugins/guests/windows/cap/change_host_name.rb @@ -1,7 +1,10 @@ +require "log4r" + module VagrantPlugins module GuestWindows module Cap module ChangeHostName + MAX_REBOOT_DURATION = 120 def self.change_host_name(machine, name) change_host_name_and_wait(machine, name, machine.config.vm.graceful_halt_timeout) @@ -11,7 +14,8 @@ module VagrantPlugins # If the configured name matches the current name, then bail # We cannot use %ComputerName% because it truncates at 15 chars return if machine.communicate.test("if ([System.Net.Dns]::GetHostName() -eq '#{name}') { exit 0 } exit 1") - + @logger = Log4r::Logger.new("vagrant::windows::change_host_name") + # Rename and reboot host if rename succeeded script = <<-EOH $computer = Get-WmiObject -Class Win32_ComputerSystem @@ -27,13 +31,24 @@ module VagrantPlugins error_class: Errors::RenameComputerFailed, error_key: :rename_computer_failed) - # Don't continue until the machine has shutdown and rebooted - 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 + + wait_remaining = MAX_REBOOT_DURATION + begin + # Don't continue until the machine has shutdown and rebooted + if machine.guest.capability?(:wait_for_reboot) + machine.guest.capability(:wait_for_reboot) + else + @logger.debug("No wait_for_reboot capability, sleeping for #{sleep_timeout} instead...") + # use graceful_halt_timeout only if guest cannot wait for reboot + sleep(sleep_timeout) + end + rescue Vagrant::Errors::MachineGuestNotReady => e + raise if wait_remaining < 0 + @logger.warn("Machine not ready, cannot wait for reboot yet. Trying again") + sleep(5) + wait_remaining -= 5 + retry + end end end end diff --git a/plugins/providers/hyperv/provider.rb b/plugins/providers/hyperv/provider.rb index c1808b6dd..505564563 100644 --- a/plugins/providers/hyperv/provider.rb +++ b/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 - return nil if !network["ip"] + guest_ip = nil + + 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 diff --git a/test/unit/plugins/providers/hyperv/provider_test.rb b/test/unit/plugins/providers/hyperv/provider_test.rb index 98e9c7d96..603bd8967 100644 --- a/test/unit/plugins/providers/hyperv/provider_test.rb +++ b/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