Remove nodes by MACs only during provisioning

The thing is nodes when being provisioned have
'interfaces' field which contains all the information
about network interfaces. This field is formatted here
nailgun/nailgun/orchestrator/provisioning_serializers.py:serialize_interfaces
and we can use this field to find system in the Cobbler
with the same MAC addresses if there exist any.
However, deletion task contains nodes which
do not have 'interfaces' field. So, we can try to
find nodes by their MACs only during provisioning,
not during deletion.

Besides, this original bug #1491725 used to appear
only during provisioning and this MAC based hack should
only be used where it is really needed.

Change-Id: I05fcbb28eee5cbfc5260e3b2a4b423548952a4dd
Closes-Bug: #1492974

lib/astute/cobbler_manager.rb

@@ -47,18 +47,8 @@ module Astute
     end
 
     def remove_nodes(nodes, retries=3, interval=2)
-      nodes_to_remove = nodes.map {|node| node['slave_name']}
+      nodes_to_remove = nodes.map {|node| node['slave_name']}.uniq
-      Astute.logger.info("List of cobbler systems to remove by their 'slave_name': #{nodes_to_remove}")
+      Astute.logger.info("Total list of nodes to remove: #{nodes_to_remove.pretty_inspect}")
-      # NOTE(kozhukalov): We try to find out if there are systems
-      # in the Cobbler with the same MAC addresses. We need to remove
-      # them, otherwise Cobbler is going to throw MAC address duplication
-      # error while trying to add a new node with MAC address which is
-      # already in use.
-      nodes_to_remove_by_mac = find_system_names_by_node_macs(nodes)
-      Astute.logger.info("List of cobbler systems to remove by thier MAC addresses: #{nodes_to_remove_by_mac}")
-      nodes_to_remove += nodes_to_remove_by_mac
-      nodes_to_remove.uniq!
-      Astute.logger.info("Total list of cobbler systems to remove: #{nodes_to_remove}")
       error_nodes = nodes_to_remove
       retries.times do
         nodes_to_remove.each do |name|
@@ -76,9 +66,9 @@ module Astute
       end
     ensure
       if error_nodes.empty?
-        Astute.logger.info("Systems have been successfully removed from cobbler: #{nodes_to_remove}")
+        Astute.logger.info("Systems have been successfully removed from cobbler: #{nodes_to_remove.pretty_inspect}")
       else
-        Astute.logger.error("Cannot remove nodes from cobbler: #{error_nodes}")
+        Astute.logger.error("Cannot remove nodes from cobbler: #{error_nodes.pretty_inspect}")
       end
       sync
     end
@@ -165,14 +155,21 @@ module Astute
       existent_nodes
     end
 
-    def find_system_names_by_node_macs(nodes)
+    def get_mac_duplicate_names(nodes)
-      found_systems = []
+      mac_duplicate_names = []
       nodes.each do |node|
-        node['interfaces'].each do |iname, ihash|
+        Astute.logger.info("Trying to find MAC duplicates for node #{node['slave_name']}")
-          found_systems << @engine.system_by_mac(ihash['mac_address']) if ihash['mac_address']
+        if node['interfaces']
+          node['interfaces'].each do |iname, ihash|
+            if ihash['mac_address']
+              Astute.logger.info("Trying to find system with MAC: #{ihash['mac_address']}")
+              found_node = @engine.system_by_mac(ihash['mac_address'])
+              mac_duplicate_names << found_node['name'] if found_node
+            end
+          end
         end
       end
-      found_systems.compact.map{|s| s['name']}.uniq
+      mac_duplicate_names.uniq
     end
 
     def sync

lib/astute/provision.rb

@@ -43,7 +43,6 @@ module Astute
       result_msg = {'nodes' => []}
       begin
         prepare_nodes(reporter, task_id, engine_attrs, nodes, cobbler)
-
         failed_uids, timeouted_uids = provision_and_watch_progress(reporter,
                                                                     task_id,
                                                                     Array.new(nodes),
@@ -543,6 +542,16 @@ module Astute
       end
 
       cobbler.remove_nodes(nodes)
+
+      # NOTE(kozhukalov): We try to find out if there are systems
+      # in the Cobbler with the same MAC addresses. If so, Cobbler is going
+      # to throw MAC address duplication error. We need to remove these
+      # nodes.
+      mac_duplicate_names = cobbler.get_mac_duplicate_names(nodes)
+      if mac_duplicate_names.present?
+        cobbler.remove_nodes(mac_duplicate_names.map {|n| {'slave_name' => n}})
+      end
+
       cobbler.add_nodes(nodes)
     end
 

spec/unit/cobbler_manager_spec.rb

@@ -170,44 +170,22 @@ describe Astute::CobblerManager do
 
     it 'should try to remove nodes using cobbler engine' do
       engine.stubs(:system_exists?).returns(true).then.returns(false)
-      cobbler_manager.expects(:find_system_names_by_node_macs)
-      .with(data['nodes'])
-      .returns(data['nodes'].map{|n| n['slave_name']})
       engine.expects(:remove_system).once
       engine.expects(:sync).once
       cobbler_manager.remove_nodes(data['nodes'])
     end
     it 'should try to remove nodes three times before giving up' do
       engine.stubs(:system_exists?).returns(true)
-      cobbler_manager.expects(:find_system_names_by_node_macs)
-      .with(data['nodes'])
-      .returns(data['nodes'].map{|n| n['slave_name']})
       engine.expects(:remove_system).times(3)
       engine.expects(:sync).once
       cobbler_manager.remove_nodes(data['nodes'])
     end
     it 'should not try to remove nodes if they do not exist' do
       engine.stubs(:system_exists?).returns(false)
-      cobbler_manager.expects(:find_system_names_by_node_macs)
-      .with(data['nodes'])
-      .returns(data['nodes'].map{|n| n['slave_name']})
       engine.expects(:remove_system).never
       engine.expects(:sync).once
       cobbler_manager.remove_nodes(data['nodes'])
     end
-    it 'should try to remove also nodes found by macs' do
-      engine.stubs(:system_exists?).with(data['nodes'][0]['slave_name'])
-      .returns(true).then.returns(false)
-      engine.stubs(:system_exists?).with('node-XXX')
-      .returns(true).then.returns(false)
-      cobbler_manager.expects(:find_system_names_by_node_macs)
-      .with(data['nodes'])
-      .returns(data['nodes'].map{|n| n['slave_name']} + ['node-XXX'])
-      engine.expects(:remove_system).twice
-      engine.expects(:sync).once
-      cobbler_manager.remove_nodes(data['nodes'])
-    end
-
   end
 
   describe '#reboot_nodes' do
@@ -292,12 +270,12 @@ describe Astute::CobblerManager do
     end
   end #'get_existent_nodes'
 
-  describe '#find_system_names_by_node_macs' do
+  describe '#get_mac_duplicate_names' do
     it 'should return cobbler names of those systems that have at least one matching mac address' do
       engine.expects(:system_by_mac).with('00:00:00:00:00:00').returns({'name' => 'node-XXX'})
       engine.expects(:system_by_mac).with('00:00:00:00:00:01').returns(nil)
 
-      expect(cobbler_manager.find_system_names_by_node_macs(data['nodes']))
+      expect(cobbler_manager.get_mac_duplicate_names(data['nodes']))
         .to eql(['node-XXX'])
     end
 
@@ -305,7 +283,7 @@ describe Astute::CobblerManager do
       engine.expects(:system_by_mac).with('00:00:00:00:00:00').returns({'name' => 'node-XXX'})
       engine.expects(:system_by_mac).with('00:00:00:00:00:01').returns({'name' => 'node-XXX'})
 
-      expect(cobbler_manager.find_system_names_by_node_macs(data['nodes']))
+      expect(cobbler_manager.get_mac_duplicate_names(data['nodes']))
         .to eql(['node-XXX'])
     end
 
@@ -313,7 +291,7 @@ describe Astute::CobblerManager do
       engine.expects(:system_by_mac).with('00:00:00:00:00:00').returns(nil)
       engine.expects(:system_by_mac).with('00:00:00:00:00:01').returns(nil)
 
-      expect(cobbler_manager.find_system_names_by_node_macs(data['nodes']))
+      expect(cobbler_manager.get_mac_duplicate_names(data['nodes']))
         .to eql([])
     end
   end #'get_existent_nodes'

spec/unit/provision_spec.rb

@@ -313,6 +313,7 @@ describe Astute::Provisioner do
           @provisioner.stubs(:provision_and_watch_progress).returns([[], []])
 
           Astute::CobblerManager.any_instance.stubs(:get_existent_nodes).returns([])
+          Astute::CobblerManager.any_instance.stubs(:get_mac_duplicate_names).returns([])
           @provisioner.stubs(:change_node_type).never
           Astute::NailgunHooks.any_instance.stubs(:process).never
 
@@ -433,6 +434,9 @@ describe Astute::Provisioner do
         Astute::CobblerManager.any_instance.expects(:get_existent_nodes)
           .with(data['nodes'])
           .returns(data['nodes'])
+        Astute::CobblerManager.any_instance.expects(:get_mac_duplicate_names)
+          .with(data['nodes'])
+          .returns([])
         Astute::CobblerManager.any_instance.expects(:add_nodes)
           .with(data['nodes'])
         Astute::CobblerManager.any_instance.expects(:remove_nodes)
@@ -458,6 +462,9 @@ describe Astute::Provisioner do
         Astute::CobblerManager.any_instance.expects(:get_existent_nodes)
           .with(data['nodes'])
           .returns([])
+        Astute::CobblerManager.any_instance.expects(:get_mac_duplicate_names)
+          .with(data['nodes'])
+          .returns([])
         Astute::CobblerManager.any_instance.expects(:remove_nodes)
           .with(data['nodes'])
         Astute::CobblerManager.any_instance.expects(:add_nodes)