Reorder qemu agent usage for use with sessions (#1396)

Adjust the order of checks around use of qemu sessions to allow use of
the agent as a priority when enabled, which should remove the need to
retrieve the address from the system connection when enabled.

Additionally adjust the call to the agent to ensure it uses the default
connection to retrieve the correct domain, rather than forcing the
system connection, which will fail to find the domain if it was created
via a user session.

Add tests that validate most of this behaviour, as well as resulting in
some minor fixes around downcasing the mac address for comparisons, and
also using instance mocks with rspec instead of pure doubles to help
catch false positives where mocks are allowing calls that done exist.

Related: #1342

lib/vagrant-libvirt/config.rb

@@ -970,7 +970,7 @@ module VagrantPlugins
         # Additional QEMU commandline environment variables
         @qemu_env = {} if @qemu_env == UNSET_VALUE
 
-        @qemu_use_agent = true if @qemu_use_agent != UNSET_VALUE
+        @qemu_use_agent = false if @qemu_use_agent == UNSET_VALUE
 
         @serials = [{:type => 'pty', :source => nil}] if @serials == []
       end
@@ -986,6 +986,9 @@ module VagrantPlugins
           end
         end
 
+        unless @qemu_use_agent == true || @qemu_use_agent == false
+          errors << "libvirt.qemu_use_agent must be a boolean."
+        end
 
         if @qemu_use_agent == true
           # if qemu agent is used to optain domain ip configuration, at least

lib/vagrant-libvirt/driver.rb

@@ -98,16 +98,16 @@ module VagrantPlugins
       end
 
       def get_domain_ipaddress(machine, domain)
-        if @machine.provider_config.qemu_use_session
-          return get_ipaddress_from_system domain.mac
-        end
-
         # attempt to get ip address from qemu agent
         if @machine.provider_config.qemu_use_agent == true
           @logger.info('Get IP via qemu agent')
           return get_ipaddress_from_qemu_agent(domain, machine.id)
         end
 
+        if @machine.provider_config.qemu_use_session
+          return get_ipaddress_from_system domain.mac
+        end
+
         # Get IP address from dhcp leases table
         begin
           ip_address = get_ipaddress_from_domain(domain)
@@ -168,9 +168,9 @@ module VagrantPlugins
       def get_ipaddress_from_qemu_agent(domain, machine_id)
         ip_address = nil
         addresses = nil
-        dom = system_connection.lookup_domain_by_uuid(machine_id)
+        libvirt_domain = connection.client.lookup_domain_by_uuid(machine_id)
         begin
-          response = dom.qemu_agent_command('{"execute":"guest-network-get-interfaces"}', timeout=10)
+          response = libvirt_domain.qemu_agent_command('{"execute":"guest-network-get-interfaces"}', timeout=10)
           @logger.debug("Got Response from qemu agent")
           @logger.debug(response)
           addresses = JSON.parse(response)
@@ -180,7 +180,7 @@ module VagrantPlugins
 
         unless addresses.nil?
           addresses["return"].each{ |interface|
-            if domain.mac == interface["hardware-address"]
+            if domain.mac.downcase == interface["hardware-address"].downcase
               @logger.debug("Found mathing interface: [%s]" % interface["name"])
               if interface.has_key?("ip-addresses")
                 interface["ip-addresses"].each{ |ip|

spec/spec_helper.rb

@@ -42,4 +42,12 @@ RSpec.configure do |config|
   config.before(:suite) do
     ENV.delete('LIBVIRT_DEFAULT_URI')
   end
+
+  config.mock_with :rspec do |mocks|
+    # This option should be set when all dependencies are being loaded
+    # before a spec run, as is the case in a typical spec helper. It will
+    # cause any verifying double instantiation for a class that does not
+    # exist to raise, protecting against incorrectly spelt names.
+    mocks.verify_doubled_constant_names = true
+  end
 end

spec/support/libvirt_context.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require 'fog/libvirt'
+require 'fog/libvirt/models/compute/server'
+require 'libvirt'
 
 shared_context 'libvirt' do
   include_context 'unit'
@@ -8,7 +10,9 @@ shared_context 'libvirt' do
   let(:libvirt_context) { true                      }
   let(:id)              { 'dummy-vagrant_dummy'     }
   let(:connection)      { double('connection') }
-  let(:domain)          { double('domain') }
+  let(:domain)          { instance_double('::Fog::Libvirt::Compute::Server') }
+  let(:libvirt_client)  { instance_double('::Libvirt::Connect') }
+  let(:libvirt_domain)  { instance_double('::Libvirt::Domain') }
   let(:logger)          { double('logger') }
 
   def connection_result(options = {})
@@ -22,11 +26,10 @@ shared_context 'libvirt' do
     stub_const('::Fog::Compute', connection)
 
     # drivers also call vm_exists? during init;
-    allow(connection).to receive(:servers).with(kind_of(String))
+    allow(connection).to receive(:servers)
       .and_return(connection_result(result: nil))
 
-    # return some information for domain when needed
-    allow(domain).to receive(:mac).and_return('9C:D5:53:F1:5A:E7')
+    allow(connection).to receive(:client).and_return(libvirt_client)
 
     allow(machine).to receive(:id).and_return(id)
     allow(Log4r::Logger).to receive(:new).and_return(logger)

spec/unit/action/shutdown_domain_spec.rb

@@ -50,7 +50,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::ShutdownDomain do
       end
 
       it "should not shutdown" do
-        expect(domain).not_to receive(:shutoff)
+        expect(domain).not_to receive(:poweroff)
         subject.call(env)
       end
 

spec/unit/action/wait_till_up_spec.rb

@@ -22,6 +22,8 @@ describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do
         .and_return(driver)
       allow(driver).to receive(:get_domain).and_return(domain)
       allow(driver).to receive(:state).and_return(:running)
+      # return some information for domain when needed
+      allow(domain).to receive(:mac).and_return('9C:D5:53:F1:5A:E7')
     end
 
     context 'when machine does not exist' do

spec/unit/driver_spec.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'fog/libvirt/requests/compute/dhcp_leases'
+
 require 'spec_helper'
 require 'support/binding_proc'
 require 'support/sharedcontext'
@@ -8,6 +10,7 @@ require 'vagrant-libvirt/driver'
 
 describe VagrantPlugins::ProviderLibvirt::Driver do
   include_context 'unit'
+  include_context 'libvirt'
 
   subject { described_class.new(machine) }
 
@@ -27,6 +30,7 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
     end
     EOF
   end
+
   # need to override the default package iso_env as using a different
   # name for the test machines above.
   let(:machine)    { iso_env.machine(:test1, :libvirt) }
@@ -36,6 +40,14 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
   let(:system_connection1) { double("system connection 1") } 
   let(:system_connection2) { double("system connection 2") } 
 
+  # make it easier for distros that want to switch the default value for
+  # qemu_use_session to true by ensuring it is explicitly false for tests.
+  before do
+    allow(machine.provider_config).to receive(:qemu_use_session).and_return(false)
+    allow(logger).to receive(:info)
+    allow(logger).to receive(:debug)
+  end
+
   describe '#connection' do
     it 'should configure a separate connection per machine' do
       expect(Fog::Compute).to receive(:new).with(
@@ -84,6 +96,129 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
     end
   end
 
+  describe '#get_ipaddress' do
+    context 'when domain exists' do
+      # not used yet, but this is the form that is returned from addresses
+      let(:addresses) { {
+        :public => ["192.168.122.111"],
+        :private => ["192.168.122.111"],
+      } }
+
+      before do
+        allow(subject).to receive(:get_domain).and_return(domain)
+      end
+
+      it 'should retrieve the address via domain fog-libvirt API' do
+        # ideally should be able to yield a block to wait_for and check that
+        # the 'addresses' function on the domain is called correctly.
+        expect(domain).to receive(:wait_for).and_return(nil)
+        expect(subject.get_ipaddress(machine)).to eq(nil)
+      end
+
+      context 'when qemu_use_agent is enabled' do
+        let(:qemu_agent_interfaces) {
+          <<-EOF
+          {
+            "return": [
+              {
+                "name": "lo",
+                "ip-addresses": [
+                  {
+                    "ip-address-type": "ipv4",
+                    "ip-address": "127.0.0.1",
+                    "prefix": 8
+                  }
+                ],
+                "hardware-address": "00:00:00:00:00:00"
+              },
+              {
+                "name": "eth0",
+                "ip-addresses": [
+                  {
+                    "ip-address-type": "ipv4",
+                    "ip-address": "192.168.122.42",
+                    "prefix": 24
+                  }
+                ],
+                "hardware-address": "52:54:00:f8:67:98"
+              }
+            ]
+          }
+          EOF
+        }
+
+        before do
+          allow(machine.provider_config).to receive(:qemu_use_agent).and_return(true)
+        end
+
+        it 'should retrieve the address via the agent' do
+          expect(subject).to receive(:connection).and_return(connection)
+          expect(libvirt_client).to receive(:lookup_domain_by_uuid).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:qemu_agent_command).and_return(qemu_agent_interfaces)
+          expect(domain).to receive(:mac).and_return("52:54:00:f8:67:98").exactly(2).times
+
+          expect(subject.get_ipaddress(machine)).to eq("192.168.122.42")
+        end
+
+        context 'when qemu_use_session is enabled' do
+          before do
+            allow(machine.provider_config).to receive(:qemu_use_session).and_return(true)
+          end
+
+          it 'should still retrieve the address via the agent' do
+            expect(subject).to receive(:connection).and_return(connection)
+            expect(libvirt_client).to receive(:lookup_domain_by_uuid).and_return(libvirt_domain)
+            expect(libvirt_domain).to receive(:qemu_agent_command).and_return(qemu_agent_interfaces)
+            expect(domain).to receive(:mac).and_return("52:54:00:f8:67:98").exactly(2).times
+
+            expect(subject.get_ipaddress(machine)).to eq("192.168.122.42")
+          end
+        end
+      end
+
+      context 'when qemu_use_session is enabled' do
+        let(:networks) { [instance_double('::Fog::Libvirt::Compute::Real')] }
+        let(:dhcp_leases) {
+          {
+            "iface"      =>"virbr0",
+            "expirytime" =>1636287162,
+            "type"       =>0,
+            "mac"        =>"52:54:00:8b:dc:5f",
+            "ipaddr"     =>"192.168.122.43",
+            "prefix"     =>24,
+            "hostname"   =>"vagrant-default_test",
+            "clientid"   =>"ff:00:8b:dc:5f:00:01:00:01:29:1a:65:42:52:54:00:8b:dc:5f",
+          }
+        }
+
+        before do
+          allow(machine.provider_config).to receive(:qemu_use_session).and_return(true)
+        end
+
+        it 'should retreive the address via the system dhcp-leases API' do
+          expect(domain).to receive(:mac).and_return("52:54:00:8b:dc:5f")
+          expect(subject).to receive(:system_connection).and_return(system_connection1)
+          expect(system_connection1).to receive(:list_all_networks).and_return(networks)
+          expect(networks[0]).to receive(:dhcp_leases).and_return([dhcp_leases])
+
+          expect(subject.get_ipaddress(machine)).to eq("192.168.122.43")
+        end
+
+        context 'when qemu_use_agent is enabled' do
+          before do
+            allow(machine.provider_config).to receive(:qemu_use_agent).and_return(true)
+          end
+
+          it 'should retrieve the address via the agent' do
+            expect(subject).to receive(:get_ipaddress_from_qemu_agent).and_return("192.168.122.44")
+
+            expect(subject.get_ipaddress(machine)).to eq("192.168.122.44")
+          end
+        end
+      end
+    end
+  end
+
   describe '#state' do
     let(:domain) { double('domain') }