From a717657232721a7fafc67ff5e1c696c9dbeb0b95 Mon Sep 17 00:00:00 2001 From: Vladimir Kozhukalov Date: Thu, 3 Sep 2015 17:58:23 +0300 Subject: [PATCH] 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 | 35 ++++++++++++++----------------- lib/astute/provision.rb | 11 +++++++++- spec/unit/cobbler_manager_spec.rb | 30 ++++---------------------- spec/unit/provision_spec.rb | 7 +++++++ 4 files changed, 37 insertions(+), 46 deletions(-) diff --git a/lib/astute/cobbler_manager.rb b/lib/astute/cobbler_manager.rb index d5a7e709..76727738 100644 --- a/lib/astute/cobbler_manager.rb +++ b/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']} - Astute.logger.info("List of cobbler systems to remove by their 'slave_name': #{nodes_to_remove}") - # 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}") + nodes_to_remove = nodes.map {|node| node['slave_name']}.uniq + Astute.logger.info("Total list of nodes to remove: #{nodes_to_remove.pretty_inspect}") 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) - found_systems = [] + def get_mac_duplicate_names(nodes) + mac_duplicate_names = [] nodes.each do |node| - node['interfaces'].each do |iname, ihash| - found_systems << @engine.system_by_mac(ihash['mac_address']) if ihash['mac_address'] + Astute.logger.info("Trying to find MAC duplicates for node #{node['slave_name']}") + 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 diff --git a/lib/astute/provision.rb b/lib/astute/provision.rb index 273576e1..0fcf372b 100644 --- a/lib/astute/provision.rb +++ b/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 diff --git a/spec/unit/cobbler_manager_spec.rb b/spec/unit/cobbler_manager_spec.rb index 3cfc4e13..88ae6e7a 100644 --- a/spec/unit/cobbler_manager_spec.rb +++ b/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' diff --git a/spec/unit/provision_spec.rb b/spec/unit/provision_spec.rb index bc11e8ab..763a73e8 100644 --- a/spec/unit/provision_spec.rb +++ b/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)