Merge pull request #23609 from kbrock/report_requires3_vm

Update Vm#find_all_by_mac_address_and_hostname_and_ipaddress

Author: Fryguy

app/models/vm.rb

@@ -44,38 +44,6 @@ def enforce_single_service_parent?
     true
   end
 
-  def self.find_all_by_mac_address_and_hostname_and_ipaddress(mac_address, hostname, ipaddress)
-    return [] if mac_address.blank? && hostname.blank? && ipaddress.blank?
-
-    include = [:vm_or_template]
-    references = []
-    conds = [["hardwares.vm_or_template_id IS NOT NULL"]]
-    if mac_address
-      conds[0] << "guest_devices.address = ?"
-      conds << mac_address
-      include << :nics
-      references << :guest_devices
-    end
-    if hostname
-      conds[0] << "networks.hostname = ?"
-      conds << hostname
-      include << :networks
-      references << :networks
-    end
-    if ipaddress
-      conds[0] << "networks.ipaddress = ?"
-      conds << ipaddress
-      include << :networks
-      references << :networks
-    end
-    conds[0] = "(#{conds[0].join(" AND ")})"
-
-    Hardware.includes(include.uniq)
-            .references(references.uniq)
-            .where(conds)
-            .map(&:vm_or_template).select { |vm| vm.kind_of?(Vm) }
-  end
-
   def running_processes
     pl = {}
     if (reason = unsupported_reason(:collect_running_processes))

lib/workers/evm_server.rb

@@ -186,15 +186,27 @@ def save_local_network_info
 
   def set_local_server_vm
     if @current_server.vm_id.nil?
-      vms = Vm.find_all_by_mac_address_and_hostname_and_ipaddress(@current_server.mac_address, @current_server.hostname, @current_server.ipaddress)
-      if vms.length > 1
-        _log.warn("Found multiple Vms that may represent this MiqServer: #{vms.collect(&:id).sort.inspect}")
-      elsif vms.length == 1
-        @current_server.update(:vm_id => vms.first.id)
+      vm_ids = find_vms_by_mac_address_and_hostname_and_ipaddress(@current_server.mac_address, @current_server.hostname, @current_server.ipaddress).pluck(:id)
+      if vm_ids.length > 1
+        _log.warn("Found multiple Vms that may represent this MiqServer: #{vm_ids.sort.inspect}")
+      elsif vm_ids.length == 1
+        @current_server.update(:vm_id => vm_ids.first)
       end
     end
   end
 
+  def find_vms_by_mac_address_and_hostname_and_ipaddress(mac_address, hostname, ipaddress)
+    return Vm.none if mac_address.blank? && hostname.blank? && ipaddress.blank?
+
+    scope = Vm
+    # NOTE: nics are guest devices with an extra where()
+    scope = scope.joins(:hardware => :nics).where(:guest_devices => {:address => mac_address}) if mac_address
+    scope = scope.joins(:hardware => :networks).where(:networks => {:hostname => hostname}) if hostname
+    scope = scope.joins(:hardware => :networks).where(:networks => {:ipaddress => ipaddress}) if ipaddress
+
+    scope
+  end
+
   def reset_server_runtime_info
     @current_server.update(
       :drb_uri        => nil,

spec/lib/workers/evm_server_spec.rb

@@ -188,4 +188,130 @@
       end
     end
   end
+
+  let(:hostname) { "ABCDEFG" }
+  let(:address) { "AB:CD:EF:GH" }
+  let(:vm1_network) { FactoryBot.create(:network, :hostname => hostname) }
+  let(:vm2_network) { FactoryBot.create(:network) }
+  let(:vm3_device) { FactoryBot.create(:guest_device, :address => address, :device_type => "ethernet") }
+  let(:vm1) { FactoryBot.create(:vm_vmware, :hardware => FactoryBot.create(:hardware, :networks => [vm1_network])) }
+  let(:vm2) { FactoryBot.create(:vm_vmware, :hardware => FactoryBot.create(:hardware, :networks => [vm2_network])) }
+  let(:vm3) { FactoryBot.create(:vm_vmware, :hardware => FactoryBot.create(:hardware, :guest_devices => [vm3_device])) }
+
+  describe "#set_local_vserver_vm (private)" do
+    let!(:miq_server) { EvmSpecHelper.local_miq_server }
+    let(:evm_server) { EvmServer.new }
+
+    # Basically stock, just ensure podified environment doesn't modify this test.
+    before { allow(evm_server).to receive(:servers_from_db).and_return([miq_server]) }
+
+    it "handles no matching servers" do
+      vm1
+      vm2
+
+      # sets evm_server.current_server
+      evm_server.send(:impersonate_server, miq_server)
+      evm_server.send(:set_local_server_vm)
+      expect(miq_server.reload.vm_id).to eq(nil)
+    end
+
+    it "finds local server (by mac_address)" do
+      vm1
+      vm2
+      miq_server.update(:hostname => vm1_network.hostname)
+
+      evm_server.send(:impersonate_server, miq_server)
+      evm_server.send(:set_local_server_vm)
+      expect(miq_server.reload.vm_id).to eq(vm1.id)
+    end
+
+    it "finds multiple servers" do
+      vm1
+      vm2
+      vm3
+
+      miq_server.update(:hostname => vm1_network.hostname, :ipaddress => vm2_network.ipaddress)
+
+      evm_server.send(:impersonate_server, miq_server)
+      evm_server.send(:set_local_server_vm)
+      expect(miq_server.reload.vm_id).to eq(nil)
+    end
+  end
+
+  context "#find_vms_by_mac_address_and_hostname_and_ipaddress (private)" do
+    subject { described_class.new }
+
+    it "mac_address" do
+      vm1
+      vm2
+      vm3
+      subject
+
+      expect do
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, address, nil, nil)).to eq([vm3])
+      end.to make_database_queries(:count => 1)
+    end
+
+    it "hostname" do
+      vm1
+      vm2
+      subject
+
+      expect do
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, nil, hostname, nil)).to eq([vm1])
+      end.to make_database_queries(:count => 1)
+    end
+
+    it "ipaddress" do
+      vm1
+      vm2
+      subject
+
+      expect do
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, nil, nil, vm1_network.ipaddress)).to eq([vm1])
+      end.to make_database_queries(:count => 1)
+    end
+
+    it "hostname and ipaddress" do
+      vm1
+      vm2
+      subject
+
+      expect do
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, nil, vm1_network.hostname, vm1_network.ipaddress)).to eq([vm1])
+      end.to make_database_queries(:count => 1)
+    end
+
+    it "hostname and different ipaddress" do
+      vm1
+      vm2
+      subject
+
+      expect do
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, nil, vm1_network.hostname, vm2_network.ipaddress)).to be_empty
+      end.to make_database_queries(:count => 1)
+    end
+
+    # vm must match both mac address and a hostname from a single server
+    # not sure if that was the original intent, but how the code currently works
+    it "mac address and different hostname" do
+      vm1
+      vm3
+      subject
+
+      expect do
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, address, vm1_network.hostname, nil)).to be_empty
+      end.to make_database_queries(:count => 1)
+    end
+
+    it "returns an empty list when all are blank" do
+      vm1
+      subject
+
+      expect do
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, nil, nil, nil)).to eq([])
+        expect(subject.send(:find_vms_by_mac_address_and_hostname_and_ipaddress, '', '', '')).to eq([])
+      end.not_to make_database_queries
+    end
+  end
 end

spec/models/vm_spec.rb

@@ -48,48 +48,6 @@
     expect { vm.validate_native_console_support }.to raise_error MiqException::RemoteConsoleNotSupportedError
   end
 
-  context ".find_all_by_mac_address_and_hostname_and_ipaddress" do
-    before do
-      @hardware1 = FactoryBot.create(:hardware)
-      @vm1 = FactoryBot.create(:vm_vmware, :hardware => @hardware1)
-
-      @hardware2 = FactoryBot.create(:hardware)
-      @vm2 = FactoryBot.create(:vm_vmware, :hardware => @hardware2)
-    end
-
-    it "mac_address" do
-      address = "ABCDEFG"
-      guest_device = FactoryBot.create(:guest_device, :address => address, :device_type => "ethernet")
-      @hardware1.guest_devices << guest_device
-
-      expect(described_class.find_all_by_mac_address_and_hostname_and_ipaddress(address, nil, nil))
-        .to eql([@vm1])
-    end
-
-    it "hostname" do
-      hostname = "ABCDEFG"
-      network = FactoryBot.create(:network, :hostname => hostname)
-      @hardware1.networks << network
-
-      expect(described_class.find_all_by_mac_address_and_hostname_and_ipaddress(nil, hostname, nil))
-        .to eql([@vm1])
-    end
-
-    it "ipaddress" do
-      ipaddress = "127.0.0.1"
-      network = FactoryBot.create(:network, :ipaddress => ipaddress)
-      @hardware1.networks << network
-
-      expect(described_class.find_all_by_mac_address_and_hostname_and_ipaddress(nil, nil, ipaddress))
-        .to eql([@vm1])
-    end
-
-    it "returns an empty list when all are blank" do
-      expect(described_class.find_all_by_mac_address_and_hostname_and_ipaddress(nil, nil, nil)).to eq([])
-      expect(described_class.find_all_by_mac_address_and_hostname_and_ipaddress('', '', '')).to eq([])
-    end
-  end
-
   context "with relationships of multiple types" do
     before do
       @rp        = FactoryBot.create(:resource_pool, :name => "RP")