Fixes #10546 - Reduce the number of interfaces duplicates

app/models/host/base.rb

@@ -171,22 +171,7 @@ def set_interfaces(parser)
       end
 
       parser.interfaces.each do |name, attributes|
-        begin
-          macaddress = Net::Validations.normalize_mac(attributes[:macaddress])
-        rescue ArgumentError
-          logger.debug "invalid mac during parsing: #{attributes[:macaddress]}"
-        end
-        base = self.interfaces.where(:mac => macaddress)
-
-        if attributes[:virtual]
-          # for virtual devices we don't check only mac address since it's not unique,
-          # if we want to update the device it must have same identifier
-          base = base.virtual.where(:identifier => name)
-        else
-          base = base.physical
-        end
-
-        iface = base.first || interface_class(name).new(:managed => false)
+        iface = get_interface_scope(name, attributes).first || interface_class(name).new(:managed => false)
         # create or update existing interface
         set_interface(attributes, name, iface)
       end
@@ -331,6 +316,30 @@ def build_required_interfaces(attrs = {})
       self.primary_interface.provision = true if self.provision_interface.nil?
     end
 
+    def get_interface_scope(name, attributes, base = self.interfaces)
+      case interface_class(name).to_s
+        # we search bonds based on identifiers, e.g. ubuntu sets random MAC after each reboot se we can't
+        # rely on mac
+        when 'Nic::Bond'
+          base.virtual.where(:identifier => name)
+        # for other interfaces we distinguish between virtual and physical interfaces
+        # for virtual devices we don't check only mac address since it's not unique,
+        # if we want to update the device it must have same identifier
+        else
+          begin
+            macaddress = Net::Validations.normalize_mac(attributes[:macaddress])
+          rescue ArgumentError
+            logger.debug "invalid mac during parsing: #{attributes[:macaddress]}"
+          end
+          base = base.where(:mac => macaddress)
+          if attributes[:virtual]
+            base.virtual.where(:identifier => name)
+          else
+            base.physical
+          end
+      end
+    end
+
     def set_interface(attributes, name, iface)
       attributes = attributes.clone
       iface.mac = attributes.delete(:macaddress)
@@ -341,7 +350,7 @@ def set_interface(attributes, name, iface)
       iface.link = attributes.delete(:link) if attributes.has_key?(:link)
       iface.identifier = name
       iface.host = self
-      update_virtuals(iface.identifier_was, name) if iface.identifier_changed? && !iface.virtual? && iface.persisted?
+      update_virtuals(iface.identifier_was, name) if iface.identifier_changed? && !iface.virtual? && iface.persisted? && iface.identifier_was.present?
       iface.attrs = attributes
 
       logger.debug "Saving #{name} NIC for host #{self.name}"

app/models/nic/base.rb

@@ -25,6 +25,8 @@ class Base < ActiveRecord::Base
               :if => Proc.new { |nic| nic.managed? && nic.host_managed? && !nic.host.compute? && !nic.virtual? }
     validates :mac, :mac_address => true, :allow_blank => true
 
+    validates :name, :uniqueness => {:scope => :host_id}
+
     # TODO uniq on primary per host
     # validate :uniq_with_hosts
 

test/unit/host_test.rb

@@ -1155,6 +1155,22 @@ def teardown
       assert_equal 'eth5', virtual.attached_to
     end
 
+    test "#set_interfaces does not update unassociated virtuals identifier on identifier change if original identifier was blank" do
+      # interface with empty identifier was renamed to eth5 (same MAC)
+      host = FactoryGirl.create(:host, :hostgroup => FactoryGirl.create(:hostgroup), :mac => '00:00:00:11:22:33')
+      host.primary_interface.update_attribute :identifier, ''
+      hash = { :bond0 => {:macaddress => '00:00:00:44:55:66', :ipaddress => '10.10.0.2', :virtual => true},
+               :eth5 => {:macaddress => '00:00:00:11:22:33', :ipaddress => '10.10.0.1', :virtual => false, :identifier => 'eth5'},
+      }.with_indifferent_access
+      parser = stub(:interfaces => hash, :ipmi_interface => {}, :suggested_primary_interface => hash.to_a.first)
+      bond0 = FactoryGirl.create(:nic_bond, :host => host, :mac => '00:00:00:44:55:66', :ip => '10.10.0.2', :identifier => 'bond0', :attached_to => '')
+
+      host.set_interfaces(parser)
+      bond0.reload
+      assert_equal 'bond0', bond0.identifier
+      assert_equal '', bond0.attached_to
+    end
+
     test "set_interfaces updates associated virtuals identifier even on primary interface" do
       host, parser = setup_host_with_nic_parser({:macaddress => '00:00:00:11:22:33', :ipaddress => '10.10.0.1', :virtual => false, :identifier => 'eth1'})
       host.primary_interface.update_attribute :identifier, 'eth0'
@@ -1167,6 +1183,23 @@ def teardown
       assert_equal 'eth1', virtual.attached_to
     end
 
+    test "#set_interfaces matches bonds based on identifier and even updates its mac" do
+      # interface with empty identifier was renamed to eth5 (same MAC)
+      host = FactoryGirl.create(:host, :hostgroup => FactoryGirl.create(:hostgroup), :mac => '00:00:00:11:22:33')
+      hash = { :bond0 => {:macaddress => 'aa:bb:cc:44:55:66', :ipaddress => '10.10.0.3', :virtual => true},
+               :eth5 => {:macaddress => '00:00:00:11:22:33', :ipaddress => '10.10.0.1', :virtual => false, :identifier => 'eth5'},
+      }.with_indifferent_access
+      parser = stub(:interfaces => hash, :ipmi_interface => {}, :suggested_primary_interface => hash.to_a.first)
+      bond0 = FactoryGirl.create(:nic_bond, :host => host, :mac => '00:00:00:44:55:66', :ip => '10.10.0.2', :identifier => 'bond0')
+
+      host.set_interfaces(parser)
+      host.interfaces.reload
+      assert_equal 1, host.interfaces.bonds.size
+      bond0.reload
+      assert_equal 'aa:bb:cc:44:55:66', bond0.mac
+      assert_equal '10.10.0.3', bond0.ip
+    end
+
     test "#set_interfaces updates associated virtuals identifier on identifier change mutualy exclusively" do
       # eth4 was renamed to eth5 and eth5 renamed to eth4
       host = FactoryGirl.create(:host, :hostgroup => FactoryGirl.create(:hostgroup))