From fa2c4258437f2053835e6a79c07c789b5ef488a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Harald=20Jens=C3=A5s?= <hjensas@redhat.com>
Date: Fri, 28 May 2021 15:14:22 +0200
Subject: [PATCH] Handle bmc-none in build_nodes_json.py

When there are less bmc_ports, compared to bm_ports filter
away bm_ports without a match bmc_port and set up the
baremetal to bmc pairs in bmc_bm_port_pairs.

All bm_ports are returned, and used with the new separate
function to build network_details. The new bmc_bm_port_pairs
is used with the _build_nodes method to create the a "nodes"
section containing only the bm nodes with a bmc port peer.

This is useful when using multiple roles, where one role
is hosting extra nodes that is not intended as virtual
baremetal nodes.

Change-Id: Ifc36d15b72c7421d7e0ec810d1ead17f4232b3ab
---
 .../build_nodes_json.py                       | 106 +++++++-----
 .../tests/test_build_nodes_json.py            | 155 +++++++++---------
 2 files changed, 144 insertions(+), 117 deletions(-)

diff --git a/openstack_virtual_baremetal/build_nodes_json.py b/openstack_virtual_baremetal/build_nodes_json.py
index 3c575eb..b011882 100755
--- a/openstack_virtual_baremetal/build_nodes_json.py
+++ b/openstack_virtual_baremetal/build_nodes_json.py
@@ -109,13 +109,23 @@ def _get_clients():
 def _get_ports(neutron, bmc_base, baremetal_base):
     all_ports = sorted(neutron.list_ports()['ports'], key=lambda x: x['name'])
     bmc_ports = list([p for p in all_ports
-                     if p['name'].startswith(bmc_base)])
+                      if p['name'].startswith(bmc_base)])
     bm_ports = list([p for p in all_ports
-                    if p['name'].startswith(baremetal_base)])
-    if len(bmc_ports) != len(bm_ports):
+                     if p['name'].startswith(baremetal_base)])
+
+    # Filter the bm_ports without a matching bmc_port, when using roles
+    # one role can have bmc enabled, and other roles have it disabled.
+    valid_suffixes = [p['name'].split(bmc_base)[-1] for p in bmc_ports]
+    valid_bm_ports = [p for p in bm_ports
+                      if p['name'].split(baremetal_base)[-1] in valid_suffixes]
+
+    if len(bmc_ports) != len(valid_bm_ports):
         raise RuntimeError('Found different numbers of baremetal and '
                            'bmc ports. bmc: %s baremetal: %s' % (bmc_ports,
                                                                  bm_ports))
+
+    bmc_bm_port_pairs = zip(bmc_ports, valid_bm_ports)
+
     provision_net_map = {}
     for port in bm_ports:
         provision_net_map.update({
@@ -123,11 +133,49 @@ def _get_ports(neutron, bmc_base, baremetal_base):
                 neutron.list_subnets(
                     id=port['fixed_ips'][0]['subnet_id'])['subnets'][0].get(
                     'name')})
-    return bmc_ports, bm_ports, provision_net_map
+
+    return bm_ports, bmc_bm_port_pairs, provision_net_map
 
 
-def _build_nodes(nova, glance, bmc_ports, bm_ports, provision_net_map,
-                 baremetal_base, undercloud_name, args):
+def _build_network_details(nova, bm_ports, undercloud_name):
+    network_details = {}
+
+    for baremetal_port in bm_ports:
+        baremetal = nova.servers.get(baremetal_port['device_id'])
+        network_details[baremetal.name] = {}
+        network_details[baremetal.name]['id'] = baremetal.id
+        network_details[baremetal.name]['ips'] = baremetal.addresses
+
+    extra_nodes = []
+
+    if undercloud_name:
+        undercloud_node_template = {
+            'name': undercloud_name,
+            'id': '',
+            'ips': [],
+        }
+        try:
+            undercloud_instance = nova.servers.list(
+                search_opts={'name': undercloud_name})[0]
+        except IndexError:
+            print('Undercloud %s specified in the environment file is not '
+                  'available in nova. No undercloud details will be '
+                  'included in the output.' % undercloud_name)
+        else:
+            undercloud_node_template['id'] = undercloud_instance.id
+            undercloud_node_template['ips'] = nova.servers.ips(
+                undercloud_instance)
+
+            extra_nodes.append(undercloud_node_template)
+            network_details[undercloud_name] = dict(
+                id=undercloud_instance.id,
+                ips=undercloud_instance.addresses)
+
+    return extra_nodes, network_details
+
+
+def _build_nodes(nova, glance, bmc_bm_port_pairs, provision_net_map,
+                 baremetal_base, args):
     node_template = {
         'pm_type': args.driver,
         'cpu': '',
@@ -142,12 +190,8 @@ def _build_nodes(nova, glance, bmc_ports, bm_ports, provision_net_map,
     }
     nodes = []
     cache = {}
-    network_details = {}
-    for bmc_port, baremetal_port in zip(bmc_ports, bm_ports):
+    for bmc_port, baremetal_port in bmc_bm_port_pairs:
         baremetal = nova.servers.get(baremetal_port['device_id'])
-        network_details[baremetal.name] = {}
-        network_details[baremetal.name]['id'] = baremetal.id
-        network_details[baremetal.name]['ips'] = baremetal.addresses
         node = dict(node_template)
         node['pm_addr'] = bmc_port['fixed_ips'][0]['ip_address']
         provision_net = provision_net_map.get(baremetal_port['id'])
@@ -201,31 +245,7 @@ def _build_nodes(nova, glance, bmc_ports, bm_ports, provision_net_map,
 
         nodes.append(node)
 
-    extra_nodes = []
-
-    if undercloud_name:
-        undercloud_node_template = {
-            'name': undercloud_name,
-            'id': '',
-            'ips': [],
-        }
-        try:
-            undercloud_instance = nova.servers.list(
-                search_opts={'name': undercloud_name})[0]
-        except IndexError:
-            print('Undercloud %s specified in the environment file is not '
-                  'available in nova. No undercloud details will be '
-                  'included in the output.' % undercloud_name)
-        else:
-            undercloud_node_template['id'] = undercloud_instance.id
-            undercloud_node_template['ips'] = nova.servers.ips(
-                undercloud_instance)
-
-            extra_nodes.append(undercloud_node_template)
-            network_details[undercloud_name] = dict(
-                id=undercloud_instance.id,
-                ips=undercloud_instance.addresses)
-    return nodes, extra_nodes, network_details
+    return nodes
 
 
 def _write_nodes(nodes, extra_nodes, network_details, args):
@@ -274,13 +294,13 @@ def main():
     args = _parse_args()
     bmc_base, baremetal_base, undercloud_name = _get_names(args)
     nova, neutron, glance = _get_clients()
-    bmc_ports, bm_ports, provision_net_map = _get_ports(neutron, bmc_base,
-                                                        baremetal_base)
-    (nodes,
-     extra_nodes,
-     network_details) = _build_nodes(nova, glance, bmc_ports, bm_ports,
-                                     provision_net_map, baremetal_base,
-                                     undercloud_name, args)
+    (bm_ports,
+     bmc_bm_port_pairs,
+     provision_net_map) = _get_ports(neutron, bmc_base, baremetal_base)
+    (extra_nodes,
+     network_details) = _build_network_details(nova, bm_ports, undercloud_name)
+    nodes = _build_nodes(nova, glance, bmc_bm_port_pairs, provision_net_map,
+                         baremetal_base, args)
     _write_nodes(nodes, extra_nodes, network_details, args)
     _write_role_nodes(nodes, args)
 
diff --git a/openstack_virtual_baremetal/tests/test_build_nodes_json.py b/openstack_virtual_baremetal/tests/test_build_nodes_json.py
index 1e45fd9..67740a2 100644
--- a/openstack_virtual_baremetal/tests/test_build_nodes_json.py
+++ b/openstack_virtual_baremetal/tests/test_build_nodes_json.py
@@ -200,12 +200,15 @@ class TestBuildNodesJson(testtools.TestCase):
         }
         neutron.list_ports.return_value = fake_ports
         neutron.list_subnets.return_value = fake_subnets
-        bmc_ports, bm_ports, provision_net_map = build_nodes_json._get_ports(
+
+        (bm_ports,
+         bmc_bm_port_pairs,
+         provision_net_map) = build_nodes_json._get_ports(
             neutron, 'bmc', 'baremetal')
-        self.assertEqual([fake_ports['ports'][2], fake_ports['ports'][1]],
-                         bmc_ports)
         self.assertEqual([fake_ports['ports'][4], fake_ports['ports'][3]],
                          bm_ports)
+        self.assertEqual([fake_ports['ports'][2], fake_ports['ports'][1]],
+                         [bmc_port for bmc_port, bm_port in bmc_bm_port_pairs])
         self.assertEqual({'baremetal_0_id': 'provision',
                           'baremetal_1_id': 'provision'}, provision_net_map)
 
@@ -246,10 +249,14 @@ class TestBuildNodesJson(testtools.TestCase):
         }
         neutron.list_ports.return_value = fake_ports
         neutron.list_subnets.return_value = fake_subnets
-        bmc_ports, bm_ports, provision_net_map = build_nodes_json._get_ports(
-            neutron, 'bmc-foo', 'baremetal-foo')
-        self.assertEqual([fake_ports['ports'][1]], bmc_ports)
+        (bm_ports,
+         bmc_bm_port_pairs,
+         provision_net_map) = build_nodes_json._get_ports(
+            neutron, '<bmc-foo', 'baremetal-foo')
         self.assertEqual([fake_ports['ports'][3]], bm_ports)
+        for bmc_port, bm_port in bmc_bm_port_pairs:
+            self.assertEqual([fake_ports['ports'][1]], bmc_port)
+            self.assertEqual([fake_ports['ports'][3]], bm_port)
 
     def _fake_port(self, device_id, ip, mac):
         return {'device_id': device_id,
@@ -279,6 +286,42 @@ class TestBuildNodesJson(testtools.TestCase):
         mock_flavor.disk = 1024
         nova.flavors.get.return_value = mock_flavor
 
+    @mock.patch('os_client_config.make_client')
+    def test_build_network_details(self, mock_make_client):
+        bm_ports = [{'device_id': '1', 'id': 'port_id_server1'},
+                    {'device_id': '2', 'id': 'port_id_server2'}]
+        nova = mock.Mock()
+        servers = [mock.Mock(), mock.Mock(), mock.Mock()]
+        self._create_build_nodes_mocks(nova, servers)
+        servers[1].image = None
+        mock_to_dict = {'os-extended-volumes:volumes_attached':
+                        [{'id': 'v0lume'}]}
+        servers[1].to_dict.return_value = mock_to_dict
+        mock_cinder = mock.Mock()
+        mock_make_client.return_value = mock_cinder
+        mock_vol = mock.Mock()
+        mock_vol.size = 100
+        mock_cinder.volumes.get.return_value = mock_vol
+        servers[2].name = 'undercloud'
+        servers[2].flavor = {'id': '1'}
+        servers[2].addresses = {'provision': [{'OS-EXT-IPS-MAC:mac_addr':
+                                              'aa:aa:aa:aa:aa:ac'}]}
+        servers[2].image = {'id': 'f00'}
+        nova.servers.list.return_value = [servers[2]]
+        ips_return_val = 'ips call value'
+        nova.servers.ips.return_value = ips_return_val
+
+        (extra_nodes,
+         network_details) = build_nodes_json._build_network_details(
+            nova, bm_ports, 'undercloud')
+
+        self.assertEqual(1, len(extra_nodes))
+        self.assertEqual('undercloud', extra_nodes[0]['name'])
+        self.assertEqual(
+            '2.1.1.1', network_details['bm_0']['ips']['provision'][0]['addr'])
+        self.assertEqual(
+            '2.1.1.2', network_details['bm_1']['ips']['provision'][0]['addr'])
+
     @mock.patch('os_client_config.make_client')
     def test_build_nodes(self, mock_make_client):
         args = mock.Mock()
@@ -317,20 +360,12 @@ class TestBuildNodesJson(testtools.TestCase):
 
         glance = mock.Mock()
 
-        (nodes,
-         extra_nodes,
-         network_details) = build_nodes_json._build_nodes(
-            nova, glance, bmc_ports, bm_ports, provision_net_map, 'bm',
-            'undercloud', args)
+        nodes = build_nodes_json._build_nodes(
+            nova, glance, zip(bmc_ports, bm_ports), provision_net_map, 'bm',
+            args)
         expected_nodes = copy.deepcopy(TEST_NODES)
         expected_nodes[1]['disk'] = 100
         self.assertEqual(expected_nodes, nodes)
-        self.assertEqual(1, len(extra_nodes))
-        self.assertEqual('undercloud', extra_nodes[0]['name'])
-        self.assertEqual(
-            '2.1.1.1', network_details['bm_0']['ips']['provision'][0]['addr'])
-        self.assertEqual(
-            '2.1.1.2', network_details['bm_1']['ips']['provision'][0]['addr'])
 
     @mock.patch('os_client_config.make_client')
     def test_build_nodes_with_driver(self, mock_make_client):
@@ -370,22 +405,14 @@ class TestBuildNodesJson(testtools.TestCase):
 
         glance = mock.Mock()
 
-        (nodes,
-         extra_nodes,
-         network_details) = build_nodes_json._build_nodes(
-            nova, glance, bmc_ports, bm_ports, provision_net_map, 'bm',
-            'undercloud', args)
+        nodes = build_nodes_json._build_nodes(
+            nova, glance, zip(bmc_ports, bm_ports), provision_net_map, 'bm',
+            args)
         expected_nodes = copy.deepcopy(TEST_NODES)
         expected_nodes[1]['disk'] = 100
         for node in expected_nodes:
             node['pm_type'] = 'ipmi'
         self.assertEqual(expected_nodes, nodes)
-        self.assertEqual(1, len(extra_nodes))
-        self.assertEqual('undercloud', extra_nodes[0]['name'])
-        self.assertEqual(
-            '2.1.1.1', network_details['bm_0']['ips']['provision'][0]['addr'])
-        self.assertEqual(
-            '2.1.1.2', network_details['bm_1']['ips']['provision'][0]['addr'])
 
     def test_build_nodes_role_uefi(self):
         args = mock.Mock()
@@ -415,9 +442,9 @@ class TestBuildNodesJson(testtools.TestCase):
         mock_image_get.get.return_value = 'uefi'
         glance.images.get.return_value = mock_image_get
 
-        nodes, extra_nodes, _ = build_nodes_json._build_nodes(
-            nova, glance, bmc_ports, bm_ports, provision_net_map, 'bm-foo',
-            None, args)
+        nodes = build_nodes_json._build_nodes(
+            nova, glance, zip(bmc_ports, bm_ports), provision_net_map,
+            'bm-foo', args)
         expected_nodes = copy.deepcopy(TEST_NODES)
         expected_nodes[0]['name'] = 'bm-foo-control-0'
         expected_nodes[0]['capabilities'] = ('boot_option:local,'
@@ -467,11 +494,9 @@ class TestBuildNodesJson(testtools.TestCase):
 
         glance = mock.Mock()
 
-        (nodes,
-         extra_nodes,
-         network_details) = build_nodes_json._build_nodes(
-            nova, glance, bmc_ports, bm_ports, provision_net_map, 'bm',
-            'undercloud', args)
+        nodes = build_nodes_json._build_nodes(
+            nova, glance, zip(bmc_ports, bm_ports), provision_net_map, 'bm',
+            args)
         expected_nodes = copy.deepcopy(TEST_NODES)
         expected_nodes[1]['disk'] = 100
         del expected_nodes[0]['ports']
@@ -479,12 +504,6 @@ class TestBuildNodesJson(testtools.TestCase):
         expected_nodes[0]['mac'] = [TEST_NODES[0]['ports'][0]['address']]
         expected_nodes[1]['mac'] = [TEST_NODES[1]['ports'][0]['address']]
         self.assertEqual(expected_nodes, nodes)
-        self.assertEqual(1, len(extra_nodes))
-        self.assertEqual('undercloud', extra_nodes[0]['name'])
-        self.assertEqual(
-            '2.1.1.1', network_details['bm_0']['ips']['provision'][0]['addr'])
-        self.assertEqual(
-            '2.1.1.2', network_details['bm_1']['ips']['provision'][0]['addr'])
 
     @mock.patch('os_client_config.make_client')
     def test_build_nodes_with_physnet(self, mock_make_client):
@@ -524,22 +543,14 @@ class TestBuildNodesJson(testtools.TestCase):
 
         glance = mock.Mock()
 
-        (nodes,
-         extra_nodes,
-         network_details) = build_nodes_json._build_nodes(
-            nova, glance, bmc_ports, bm_ports, provision_net_map, 'bm',
-            'undercloud', args)
+        nodes = build_nodes_json._build_nodes(
+            nova, glance, zip(bmc_ports, bm_ports), provision_net_map, 'bm',
+            args)
         expected_nodes = copy.deepcopy(TEST_NODES)
         expected_nodes[1]['disk'] = 100
         expected_nodes[0]['ports'][0]['physical_network'] = 'provision'
         expected_nodes[1]['ports'][0]['physical_network'] = 'provision'
         self.assertEqual(expected_nodes, nodes)
-        self.assertEqual(1, len(extra_nodes))
-        self.assertEqual('undercloud', extra_nodes[0]['name'])
-        self.assertEqual(
-            '2.1.1.1', network_details['bm_0']['ips']['provision'][0]['addr'])
-        self.assertEqual(
-            '2.1.1.2', network_details['bm_1']['ips']['provision'][0]['addr'])
 
     @mock.patch('os_client_config.make_client')
     def test_build_nodes_with_physnet_strip_id(self, mock_make_client):
@@ -580,24 +591,14 @@ class TestBuildNodesJson(testtools.TestCase):
 
         glance = mock.Mock()
 
-        (nodes,
-         extra_nodes,
-         network_details) = build_nodes_json._build_nodes(
-            nova, glance, bmc_ports, bm_ports, provision_net_map, 'bm',
-            'undercloud', args)
+        nodes = build_nodes_json._build_nodes(
+            nova, glance, zip(bmc_ports, bm_ports), provision_net_map, 'bm',
+            args)
         expected_nodes = copy.deepcopy(TEST_NODES)
         expected_nodes[1]['disk'] = 100
         expected_nodes[0]['ports'][0]['physical_network'] = 'ctlplane'
         expected_nodes[1]['ports'][0]['physical_network'] = 'ctlplane'
         self.assertEqual(expected_nodes, nodes)
-        self.assertEqual(1, len(extra_nodes))
-        self.assertEqual('undercloud', extra_nodes[0]['name'])
-        self.assertEqual(
-            '2.1.1.1',
-            network_details['bm_0']['ips']['ctlplane-123'][0]['addr'])
-        self.assertEqual(
-            '2.1.1.2',
-            network_details['bm_1']['ips']['ctlplane-123'][0]['addr'])
 
     @mock.patch('openstack_virtual_baremetal.build_nodes_json.open',
                 create=True)
@@ -662,13 +663,15 @@ class TestBuildNodesJson(testtools.TestCase):
                 '_write_role_nodes')
     @mock.patch('openstack_virtual_baremetal.build_nodes_json._write_nodes')
     @mock.patch('openstack_virtual_baremetal.build_nodes_json._build_nodes')
+    @mock.patch('openstack_virtual_baremetal.build_nodes_json.'
+                '_build_network_details')
     @mock.patch('openstack_virtual_baremetal.build_nodes_json._get_ports')
     @mock.patch('openstack_virtual_baremetal.build_nodes_json._get_clients')
     @mock.patch('openstack_virtual_baremetal.build_nodes_json._get_names')
     @mock.patch('openstack_virtual_baremetal.build_nodes_json._parse_args')
     def test_main(self, mock_parse_args, mock_get_names, mock_get_clients,
-                  mock_get_ports, mock_build_nodes, mock_write_nodes,
-                  mock_write_role_nodes):
+                  mock_get_ports, mock_build_net_details, mock_build_nodes,
+                  mock_write_nodes, mock_write_role_nodes):
         args = mock.Mock()
         mock_parse_args.return_value = args
         bmc_base = mock.Mock()
@@ -681,13 +684,15 @@ class TestBuildNodesJson(testtools.TestCase):
         neutron = mock.Mock()
         glance = mock.Mock()
         mock_get_clients.return_value = (nova, neutron, glance)
-        bmc_ports = mock.Mock()
+        bmc_bm_port_pairs = mock.Mock()
         bm_ports = mock.Mock()
-        mock_get_ports.return_value = (bmc_ports, bm_ports, provision_net_map)
+        mock_get_ports.return_value = (bm_ports, bmc_bm_port_pairs,
+                                       provision_net_map)
         nodes = mock.Mock()
         extra_nodes = mock.Mock()
         network_details = mock.Mock()
-        mock_build_nodes.return_value = (nodes, extra_nodes, network_details)
+        mock_build_net_details.return_value = (extra_nodes, network_details)
+        mock_build_nodes.return_value = nodes
 
         build_nodes_json.main()
 
@@ -696,10 +701,12 @@ class TestBuildNodesJson(testtools.TestCase):
         mock_get_clients.assert_called_once_with()
         mock_get_ports.assert_called_once_with(neutron, bmc_base,
                                                baremetal_base)
-        mock_build_nodes.assert_called_once_with(nova, glance, bmc_ports,
-                                                 bm_ports, provision_net_map,
+        mock_build_net_details.assert_called_once_with(nova, bm_ports,
+                                                       undercloud_name)
+        mock_build_nodes.assert_called_once_with(nova, glance,
+                                                 bmc_bm_port_pairs,
+                                                 provision_net_map,
                                                  baremetal_base,
-                                                 undercloud_name,
                                                  args)
         mock_write_nodes.assert_called_once_with(nodes, extra_nodes,
                                                  network_details, args)