From c8c590f586bb8f93188d75a886d9f3b79b893c6b Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Sat, 13 Mar 2021 19:55:17 +0000 Subject: [PATCH] Consolidate ip address retreival to single method (#1199) Refactor WaitTillUp action to make use of the domain IP address retrieval code in the driver to ensure a single place to maintain. Remove references to machine option for driver where already should be available as an instance variable. --- lib/vagrant-libvirt/action/wait_till_up.rb | 32 ++------- lib/vagrant-libvirt/driver.rb | 79 +++++++++++++--------- lib/vagrant-libvirt/provider.rb | 2 +- spec/unit/action/wait_till_up_spec.rb | 17 ++--- 4 files changed, 63 insertions(+), 67 deletions(-) diff --git a/lib/vagrant-libvirt/action/wait_till_up.rb b/lib/vagrant-libvirt/action/wait_till_up.rb index 30a9413..82a569d 100644 --- a/lib/vagrant-libvirt/action/wait_till_up.rb +++ b/lib/vagrant-libvirt/action/wait_till_up.rb @@ -34,33 +34,13 @@ module VagrantPlugins @logger.debug("Searching for IP for MAC address: #{domain.mac}") env[:ui].info(I18n.t('vagrant_libvirt.waiting_for_ip')) - if env[:machine].provider_config.qemu_use_session - env[:metrics]['instance_ip_time'] = Util::Timer.time do - retryable(on: Fog::Errors::TimeoutError, tries: 300) do - # just return if interrupted and let the warden call recover - return if env[:interrupted] + env[:metrics]['instance_ip_time'] = Util::Timer.time do + retryable(on: Fog::Errors::TimeoutError, tries: 300) do + # just return if interrupted and let the warden call recover + return if env[:interrupted] - # Wait for domain to obtain an ip address - domain.wait_for(2) do - env[:ip_address] = env[:machine].provider.driver.get_ipaddress_system(domain.mac) - !env[:ip_address].nil? - end - end - end - else - env[:metrics]['instance_ip_time'] = Util::Timer.time do - retryable(on: Fog::Errors::TimeoutError, tries: 300) do - # just return if interrupted and let the warden call recover - return if env[:interrupted] - - # Wait for domain to obtain an ip address - domain.wait_for(2) do - addresses.each_pair do |_type, ip| - env[:ip_address] = ip[0] unless ip[0].nil? - end - !env[:ip_address].nil? - end - end + # Wait for domain to obtain an ip address + env[:ip_address] = env[:machine].provider.driver.get_domain_ipaddress(domain) end end diff --git a/lib/vagrant-libvirt/driver.rb b/lib/vagrant-libvirt/driver.rb index 9352020..16baba3 100644 --- a/lib/vagrant-libvirt/driver.rb +++ b/lib/vagrant-libvirt/driver.rb @@ -60,12 +60,12 @@ module VagrantPlugins @@system_connection end - def get_domain(mid) + def get_domain begin - domain = connection.servers.get(mid) + domain = connection.servers.get(@machine.id) rescue Libvirt::RetrieveError => e if e.libvirt_code == ProviderLibvirt::Util::ErrorCodes::VIR_ERR_NO_DOMAIN - @logger.debug("machine #{mid} not found #{e}.") + @logger.debug("machine #{@machine.name} domain not found #{e}.") return nil else raise e @@ -80,54 +80,38 @@ module VagrantPlugins !domain.nil? end - def get_ipaddress(machine) + def get_ipaddress # Find the machine - domain = get_domain(machine.id) - if @machine.provider_config.qemu_use_session - return get_ipaddress_system domain.mac - end + domain = get_domain(@machine.id) if domain.nil? # The machine can't be found return nil end + get_domain_ipaddress(domain) + end + + def get_domain_ipaddress(domain) + if @machine.provider_config.qemu_use_session + return get_ipaddress_from_system domain.mac + end + # Get IP address from arp table - ip_address = nil begin - domain.wait_for(2) do - addresses.each_pair do |_type, ip| - # Multiple leases are separated with a newline, return only - # the most recent address - ip_address = ip[0].split("\n").first unless ip[0].nil? - end - !ip_address.nil? - end + ip_address = get_ipaddress_from_domain(domain) rescue Fog::Errors::TimeoutError - @logger.info('Timeout at waiting for an ip address for machine %s' % machine.name) + @logger.info('Timeout at waiting for an ip address for machine %s' % @machine.name) end unless ip_address - @logger.info('No arp table entry found for machine %s' % machine.name) + @logger.info('No arp table entry found for machine %s' % @machine.name) return nil end ip_address end - def get_ipaddress_system(mac) - ip_address = nil - - system_connection.list_all_networks.each do |net| - leases = net.dhcp_leases(mac, 0) - # Assume the lease expiring last is the current IP address - ip_address = leases.sort_by { |lse| lse["expirytime"] }.last["ipaddr"] if !leases.empty? - break if ip_address - end - - return ip_address - end - def state(machine) # may be other error states with initial retreival we can't handle begin @@ -142,6 +126,37 @@ module VagrantPlugins domain.state.tr('-', '_').to_sym end + + private + + def get_ipaddress_from_system(mac) + ip_address = nil + + system_connection.list_all_networks.each do |net| + leases = net.dhcp_leases(mac, 0) + # Assume the lease expiring last is the current IP address + ip_address = leases.sort_by { |lse| lse["expirytime"] }.last["ipaddr"] if !leases.empty? + break if ip_address + end + + ip_address + end + + def get_ipaddress_from_domain(domain) + ip_address = nil + domain.wait_for(2) do + addresses.each_pair do |type, ip| + # Multiple leases are separated with a newline, return only + # the most recent address + ip_address = ip[0].split("\n").first if ip[0] != nil + end + + ip_address != nil + end + + ip_address + end + end end end diff --git a/lib/vagrant-libvirt/provider.rb b/lib/vagrant-libvirt/provider.rb index 13b0e6a..6f6886f 100644 --- a/lib/vagrant-libvirt/provider.rb +++ b/lib/vagrant-libvirt/provider.rb @@ -55,7 +55,7 @@ module VagrantPlugins # be called from other threads of execution. return nil if state.id != :running - ip = driver.get_ipaddress(@machine) + ip = driver.get_ipaddress # if can't determine the IP, just return nil and let the core # deal with it, similar to the docker provider diff --git a/spec/unit/action/wait_till_up_spec.rb b/spec/unit/action/wait_till_up_spec.rb index a6acf4a..60f83f3 100644 --- a/spec/unit/action/wait_till_up_spec.rb +++ b/spec/unit/action/wait_till_up_spec.rb @@ -12,18 +12,19 @@ describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do include_context 'libvirt' include_context 'unit' + let (:driver) { double('driver') } + describe '#call' do before do - allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver) - .to receive(:get_domain).and_return(domain) - allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:state) - .and_return(:running) + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Provider).to receive(:driver) + .and_return(driver) + allow(driver).to receive(:get_domain).and_return(domain) + allow(driver).to receive(:state).and_return(:running) end context 'when machine does not exist' do before do - allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver) - .to receive(:get_domain).and_return(nil) + allow(driver).to receive(:get_domain).and_return(nil) end it 'raises exception' do @@ -51,7 +52,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do allow(domain).to receive(:wait_for).and_return(true) allow(env).to receive(:[]).and_call_original allow(env).to receive(:[]).with(:interrupted).and_return(false, true, true) - allow(env).to receive(:[]).with(:ip_address).and_return('192.168.121.2') + allow(driver).to receive(:get_domain_ipaddress).and_return('192.168.121.2') end it 'should exit after getting IP' do expect(app).to_not receive(:call) @@ -71,7 +72,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do allow(domain).to receive(:wait_for).and_return(true) allow(env).to receive(:[]).and_call_original allow(env).to receive(:[]).with(:interrupted).and_return(false) - allow(env).to receive(:[]).with(:ip_address).and_return('192.168.121.2') + allow(driver).to receive(:get_domain_ipaddress).and_return('192.168.121.2') end it 'should call the next hook' do expect(app).to receive(:call)