diff --git a/ironic/common/dhcp_factory.py b/ironic/common/dhcp_factory.py index 3bf1215149..dfd4e452b1 100644 --- a/ironic/common/dhcp_factory.py +++ b/ironic/common/dhcp_factory.py @@ -89,8 +89,18 @@ class DHCPFactory(object): 'opt_value': '123.123.123.456'}, {'opt_name': 'tftp-server', 'opt_value': '123.123.123.123'}] - :param ports: a list of Neutron port dicts to update DHCP options on. - If None, will get the list of ports from the Ironic port objects. + + :param ports: A dict with keys 'ports' and 'portgroups' and + dicts as values. Each dict has key/value pairs of the form + :. e.g. + + :: + + {'ports': {'port.uuid': vif.id}, + 'portgroups': {'portgroup.uuid': vif.id}} + + If the value is None, will get the list of ports/portgroups + from the Ironic port/portgroup objects. """ self.provider.update_dhcp_opts(task, dhcp_opts, ports) diff --git a/ironic/common/network.py b/ironic/common/network.py index de55979059..7d8b3eb567 100644 --- a/ironic/common/network.py +++ b/ironic/common/network.py @@ -19,12 +19,26 @@ def get_node_vif_ids(task): This function does not handle multi node operations. :param task: a TaskManager instance. - :returns: A dict of the Node's port UUIDs and their associated VIFs + :returns: A dict of Node's neutron ports where keys are + 'ports' & 'portgroups' and the values are dict of UUIDs + and their associated VIFs, e.g. + :: + + {'ports': {'port.uuid': vif.id}, + 'portgroups': {'portgroup.uuid': vif.id}} """ + vifs = {} + portgroup_vifs = {} port_vifs = {} + for portgroup in task.portgroups: + vif = portgroup.extra.get('vif_port_id') + if vif: + portgroup_vifs[portgroup.uuid] = vif + vifs['portgroups'] = portgroup_vifs for port in task.ports: vif = port.extra.get('vif_port_id') if vif: port_vifs[port.uuid] = vif - return port_vifs + vifs['ports'] = port_vifs + return vifs diff --git a/ironic/dhcp/base.py b/ironic/dhcp/base.py index ad1e4e0425..9ebb20f5f4 100644 --- a/ironic/dhcp/base.py +++ b/ironic/dhcp/base.py @@ -74,21 +74,27 @@ class BaseDHCP(object): {'opt_name': 'tftp-server', 'opt_value': '123.123.123.123'}] - :param vifs: a dict of Neutron port dicts to update DHCP options on. - The keys should be Ironic port UUIDs, and the values should be - Neutron port UUIDs - If the value is None, will get the list of ports from the Ironic - port objects. + :param vifs: A dict with keys 'ports' and 'portgroups' and + dicts as values. Each dict has key/value pairs of the form + :. e.g. + :: + + {'ports': {'port.uuid': vif.id}, + 'portgroups': {'portgroup.uuid': vif.id}} + + If the value is None, will get the list of ports/portgroups + from the Ironic port/portgroup objects. :raises: FailedToUpdateDHCPOptOnPort """ @abc.abstractmethod def get_ip_addresses(self, task): - """Get IP addresses for all ports in `task`. + """Get IP addresses for all ports/portgroups in `task`. - :param task: a TaskManager instance. - :returns: List of IP addresses associated with task.ports + :param task: A TaskManager instance. + :returns: List of IP addresses associated with + task's ports and portgroups. """ def clean_dhcp_opts(self, task): diff --git a/ironic/dhcp/neutron.py b/ironic/dhcp/neutron.py index 167d7ecbbe..60499dabfe 100644 --- a/ironic/dhcp/neutron.py +++ b/ironic/dhcp/neutron.py @@ -30,6 +30,7 @@ from ironic.common import keystone from ironic.common import network from ironic.dhcp import base from ironic.drivers.modules import ssh +from ironic.objects.port import Port neutron_opts = [ @@ -152,37 +153,44 @@ class NeutronDHCPApi(base.BaseDHCP): 'opt_value': '123.123.123.456'}, {'opt_name': 'tftp-server', 'opt_value': '123.123.123.123'}] - :param vifs: a dict of Neutron port dicts to update DHCP options on. - The keys should be Ironic port UUIDs, and the values should be - Neutron port UUIDs - If the value is None, will get the list of ports from the Ironic - port objects. + :param vifs: a dict of Neutron port/portgroup dicts + to update DHCP options on. The port/portgroup dict key + should be Ironic port UUIDs, and the values should be + Neutron port UUIDs, e.g. + + :: + + {'ports': {'port.uuid': vif.id}, + 'portgroups': {'portgroup.uuid': vif.id}} + If the value is None, will get the list of ports/portgroups + from the Ironic port/portgroup objects. """ if vifs is None: vifs = network.get_node_vif_ids(task) - if not vifs: + if not (vifs['ports'] or vifs['portgroups']): raise exception.FailedToUpdateDHCPOptOnPort( _("No VIFs found for node %(node)s when attempting " "to update DHCP BOOT options.") % {'node': task.node.uuid}) failures = [] - for port_id, port_vif in vifs.items(): + vif_list = [vif for pdict in vifs.values() for vif in pdict.values()] + for vif in vif_list: try: - self.update_port_dhcp_opts(port_vif, options, + self.update_port_dhcp_opts(vif, options, token=task.context.auth_token) except exception.FailedToUpdateDHCPOptOnPort: - failures.append(port_id) + failures.append(vif) if failures: - if len(failures) == len(vifs): + if len(failures) == len(vif_list): raise exception.FailedToUpdateDHCPOptOnPort(_( "Failed to set DHCP BOOT options for any port on node %s.") % task.node.uuid) else: LOG.warning(_LW("Some errors were encountered when updating " "the DHCP BOOT options for node %(node)s on " - "the following ports: %(ports)s."), + "the following Neutron ports: %(ports)s."), {'node': task.node.uuid, 'ports': failures}) # TODO(adam_g): Hack to workaround bug 1334447 until we have a @@ -195,7 +203,7 @@ class NeutronDHCPApi(base.BaseDHCP): time.sleep(15) def _get_fixed_ip_address(self, port_uuid, client): - """Get a port's fixed ip address. + """Get a Neutron port's fixed ip address. :param port_uuid: Neutron port id. :param client: Neutron client instance. @@ -231,55 +239,81 @@ class NeutronDHCPApi(base.BaseDHCP): port_uuid) raise exception.FailedToGetIPAddressOnPort(port_id=port_uuid) - def _get_port_ip_address(self, task, port_uuid, client): - """Get ip address of ironic port assigned by neutron. + def _get_port_ip_address(self, task, p_obj, client): + """Get ip address of ironic port/portgroup assigned by Neutron. :param task: a TaskManager instance. - :param port_uuid: ironic Node's port UUID. + :param p_obj: Ironic port or portgroup object. :param client: Neutron client instance. - :returns: Neutron port ip address associated with Node's port. + :returns: List of Neutron vif ip address associated with + Node's port/portgroup. :raises: FailedToGetIPAddressOnPort :raises: InvalidIPv4Address """ - vifs = network.get_node_vif_ids(task) - if not vifs: + vif = p_obj.extra.get('vif_port_id') + if not vif: + obj_name = 'portgroup' + if isinstance(p_obj, Port): + obj_name = 'port' LOG.warning(_LW("No VIFs found for node %(node)s when attempting " - " to get port IP address."), - {'node': task.node.uuid}) - raise exception.FailedToGetIPAddressOnPort(port_id=port_uuid) + "to get IP address for %(obj_name)s: %(obj_id)."), + {'node': task.node.uuid, 'obj_name': obj_name, + 'obj_id': p_obj.uuid}) + raise exception.FailedToGetIPAddressOnPort(port_id=p_obj.uuid) - port_vif = vifs[port_uuid] + vif_ip_address = self._get_fixed_ip_address(vif, client) + return vif_ip_address - port_ip_address = self._get_fixed_ip_address(port_vif, client) - return port_ip_address - - def get_ip_addresses(self, task): - """Get IP addresses for all ports in `task`. + def _get_ip_addresses(self, task, pobj_list, client): + """Get IP addresses for all ports/portgroups. :param task: a TaskManager instance. - :returns: List of IP addresses associated with task.ports. + :param pobj_list: List of port or portgroup objects. + :param client: Neutron client instance. + :returns: List of IP addresses associated with + task's ports/portgroups. """ - client = _build_client(task.context.auth_token) failures = [] ip_addresses = [] - for port in task.ports: + for obj in pobj_list: try: - port_ip_address = self._get_port_ip_address(task, port.uuid, - client) - ip_addresses.append(port_ip_address) + vif_ip_address = self._get_port_ip_address(task, obj, + client) + ip_addresses.append(vif_ip_address) except (exception.FailedToGetIPAddressOnPort, exception.InvalidIPv4Address): - failures.append(port.uuid) + failures.append(obj.uuid) if failures: - LOG.warning(_LW("Some errors were encountered on node %(node)s" - " while retrieving IP address on the following" - " ports: %(ports)s."), - {'node': task.node.uuid, 'ports': failures}) + obj_name = 'portgroups' + if isinstance(pobj_list[0], Port): + obj_name = 'ports' + + LOG.warning(_LW( + "Some errors were encountered on node %(node)s " + "while retrieving IP addresses on the following " + "%(obj_name)s: %(failures)s."), + {'node': task.node.uuid, 'obj_name': obj_name, + 'failures': failures}) return ip_addresses + def get_ip_addresses(self, task): + """Get IP addresses for all ports/portgroups in `task`. + + :param task: a TaskManager instance. + :returns: List of IP addresses associated with + task's ports/portgroups. + """ + client = _build_client(task.context.auth_token) + + port_ip_addresses = self._get_ip_addresses(task, task.ports, client) + portgroup_ip_addresses = self._get_ip_addresses( + task, task.portgroups, client) + + return port_ip_addresses + portgroup_ip_addresses + def create_cleaning_ports(self, task): """Create neutron ports for each port on task.node to boot the ramdisk. diff --git a/ironic/tests/unit/common/test_network.py b/ironic/tests/unit/common/test_network.py index 3be34006d4..8a318f2b86 100644 --- a/ironic/tests/unit/common/test_network.py +++ b/ironic/tests/unit/common/test_network.py @@ -30,8 +30,9 @@ class TestNetwork(db_base.DbTestCase): mgr_utils.mock_the_extension_manager(driver='fake') self.node = object_utils.create_test_node(self.context) - def test_get_node_vif_ids_no_ports(self): - expected = {} + def test_get_node_vif_ids_no_ports_no_portgroups(self): + expected = {'portgroups': {}, + 'ports': {}} with task_manager.acquire(self.context, self.node.uuid) as task: result = network.get_node_vif_ids(task) self.assertEqual(expected, result) @@ -42,7 +43,19 @@ class TestNetwork(db_base.DbTestCase): uuid=uuidutils.generate_uuid(), extra={'vif_port_id': 'test-vif-A'}, driver='fake') - expected = {port1.uuid: 'test-vif-A'} + expected = {'portgroups': {}, + 'ports': {port1.uuid: 'test-vif-A'}} + with task_manager.acquire(self.context, self.node.uuid) as task: + result = network.get_node_vif_ids(task) + self.assertEqual(expected, result) + + def test_get_node_vif_ids_one_portgroup(self): + pg1 = db_utils.create_test_portgroup( + node_id=self.node.id, + extra={'vif_port_id': 'test-vif-A'}) + + expected = {'portgroups': {pg1.uuid: 'test-vif-A'}, + 'ports': {}} with task_manager.acquire(self.context, self.node.uuid) as task: result = network.get_node_vif_ids(task) self.assertEqual(expected, result) @@ -58,7 +71,26 @@ class TestNetwork(db_base.DbTestCase): uuid=uuidutils.generate_uuid(), extra={'vif_port_id': 'test-vif-B'}, driver='fake') - expected = {port1.uuid: 'test-vif-A', port2.uuid: 'test-vif-B'} + expected = {'portgroups': {}, + 'ports': {port1.uuid: 'test-vif-A', + port2.uuid: 'test-vif-B'}} + with task_manager.acquire(self.context, self.node.uuid) as task: + result = network.get_node_vif_ids(task) + self.assertEqual(expected, result) + + def test_get_node_vif_ids_two_portgroups(self): + pg1 = db_utils.create_test_portgroup( + node_id=self.node.id, + extra={'vif_port_id': 'test-vif-A'}) + pg2 = db_utils.create_test_portgroup( + uuid=uuidutils.generate_uuid(), + address='dd:ee:ff:aa:bb:cc', + node_id=self.node.id, + name='barname', + extra={'vif_port_id': 'test-vif-B'}) + expected = {'portgroups': {pg1.uuid: 'test-vif-A', + pg2.uuid: 'test-vif-B'}, + 'ports': {}} with task_manager.acquire(self.context, self.node.uuid) as task: result = network.get_node_vif_ids(task) self.assertEqual(expected, result) diff --git a/ironic/tests/unit/dhcp/test_neutron.py b/ironic/tests/unit/dhcp/test_neutron.py index e3c856e332..1b0283469a 100644 --- a/ironic/tests/unit/dhcp/test_neutron.py +++ b/ironic/tests/unit/dhcp/test_neutron.py @@ -197,7 +197,8 @@ class TestNeutron(db_base.DbTestCase): @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') @mock.patch('ironic.common.network.get_node_vif_ids') def test_update_dhcp(self, mock_gnvi, mock_updo): - mock_gnvi.return_value = {'port-uuid': 'vif-uuid'} + mock_gnvi.return_value = {'ports': {'port-uuid': 'vif-uuid'}, + 'portgroups': {}} with task_manager.acquire(self.context, self.node.uuid) as task: opts = pxe_utils.dhcp_options_for_instance(task) @@ -209,7 +210,7 @@ class TestNeutron(db_base.DbTestCase): @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') @mock.patch('ironic.common.network.get_node_vif_ids') def test_update_dhcp_no_vif_data(self, mock_gnvi, mock_updo): - mock_gnvi.return_value = {} + mock_gnvi.return_value = {'portgroups': {}, 'ports': {}} with task_manager.acquire(self.context, self.node.uuid) as task: api = dhcp_factory.DHCPFactory() @@ -221,7 +222,8 @@ class TestNeutron(db_base.DbTestCase): @mock.patch('ironic.common.network.get_node_vif_ids') def test_update_dhcp_some_failures(self, mock_gnvi, mock_updo): # confirm update is called twice, one fails, but no exception raised - mock_gnvi.return_value = {'p1': 'v1', 'p2': 'v2'} + mock_gnvi.return_value = {'ports': {'p1': 'v1', 'p2': 'v2'}, + 'portgroups': {}} exc = exception.FailedToUpdateDHCPOptOnPort('fake exception') mock_updo.side_effect = [None, exc] with task_manager.acquire(self.context, @@ -235,7 +237,8 @@ class TestNeutron(db_base.DbTestCase): @mock.patch('ironic.common.network.get_node_vif_ids') def test_update_dhcp_fails(self, mock_gnvi, mock_updo): # confirm update is called twice, both fail, and exception is raised - mock_gnvi.return_value = {'p1': 'v1', 'p2': 'v2'} + mock_gnvi.return_value = {'ports': {'p1': 'v1', 'p2': 'v2'}, + 'portgroups': {}} exc = exception.FailedToUpdateDHCPOptOnPort('fake exception') mock_updo.side_effect = [exc, exc] with task_manager.acquire(self.context, @@ -307,8 +310,7 @@ class TestNeutron(db_base.DbTestCase): fake_client.show_port.assert_called_once_with(port_id) @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_fixed_ip_address') - @mock.patch('ironic.common.network.get_node_vif_ids') - def test__get_port_ip_address(self, mock_gnvi, mock_gfia): + def test__get_port_ip_address(self, mock_gfia): expected = "192.168.1.3" port = object_utils.create_test_port(self.context, node_id=self.node.id, @@ -317,29 +319,42 @@ class TestNeutron(db_base.DbTestCase): extra={'vif_port_id': 'test-vif-A'}, driver='fake') - mock_gnvi.return_value = {port.uuid: 'vif-uuid'} mock_gfia.return_value = expected with task_manager.acquire(self.context, self.node.uuid) as task: api = dhcp_factory.DHCPFactory().provider - result = api._get_port_ip_address(task, port.uuid, + result = api._get_port_ip_address(task, port, mock.sentinel.client) - mock_gnvi.assert_called_once_with(task) self.assertEqual(expected, result) - mock_gfia.assert_called_once_with('vif-uuid', mock.sentinel.client) + mock_gfia.assert_called_once_with('test-vif-A', mock.sentinel.client) @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_fixed_ip_address') - @mock.patch('ironic.common.network.get_node_vif_ids') - def test__get_port_ip_address_with_exception(self, mock_gnvi, mock_gfia): + def test__get_port_ip_address_for_portgroup(self, mock_gfia): + expected = "192.168.1.3" + pg = object_utils.create_test_portgroup(self.context, + node_id=self.node.id, + address='aa:bb:cc:dd:ee:ff', + uuid=uuidutils.generate_uuid(), + extra={'vif_port_id': + 'test-vif-A'}, + driver='fake') + mock_gfia.return_value = expected + with task_manager.acquire(self.context, + self.node.uuid) as task: + api = dhcp_factory.DHCPFactory().provider + result = api._get_port_ip_address(task, pg, + mock.sentinel.client) + self.assertEqual(expected, result) + mock_gfia.assert_called_once_with('test-vif-A', mock.sentinel.client) + + @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_fixed_ip_address') + def test__get_port_ip_address_with_exception(self, mock_gfia): expected = "192.168.1.3" port = object_utils.create_test_port(self.context, node_id=self.node.id, address='aa:bb:cc:dd:ee:ff', uuid=uuidutils.generate_uuid(), - extra={'vif_port_id': - 'test-vif-A'}, driver='fake') - mock_gnvi.return_value = None mock_gfia.return_value = expected with task_manager.acquire(self.context, self.node.uuid) as task: @@ -347,7 +362,58 @@ class TestNeutron(db_base.DbTestCase): self.assertRaises(exception.FailedToGetIPAddressOnPort, api._get_port_ip_address, task, port, mock.sentinel.client) - mock_gnvi.assert_called_once_with(task) + + @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_fixed_ip_address') + def test__get_port_ip_address_for_portgroup_with_exception( + self, mock_gfia): + expected = "192.168.1.3" + pg = object_utils.create_test_portgroup(self.context, + node_id=self.node.id, + address='aa:bb:cc:dd:ee:ff', + uuid=uuidutils.generate_uuid(), + driver='fake') + mock_gfia.return_value = expected + with task_manager.acquire(self.context, + self.node.uuid) as task: + api = dhcp_factory.DHCPFactory().provider + self.assertRaises(exception.FailedToGetIPAddressOnPort, + api._get_port_ip_address, task, pg, + mock.sentinel.client) + + @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_fixed_ip_address') + def test__get_ip_addresses_ports(self, mock_gfia): + ip_address = '10.10.0.1' + expected = [ip_address] + port = object_utils.create_test_port(self.context, + node_id=self.node.id, + address='aa:bb:cc:dd:ee:ff', + uuid=uuidutils.generate_uuid(), + extra={'vif_port_id': + 'test-vif-A'}, + driver='fake') + mock_gfia.return_value = ip_address + with task_manager.acquire(self.context, self.node.uuid) as task: + api = dhcp_factory.DHCPFactory().provider + result = api._get_ip_addresses(task, [port], + mock.sentinel.client) + self.assertEqual(expected, result) + + @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_fixed_ip_address') + def test__get_ip_addresses_portgroup(self, mock_gfia): + ip_address = '10.10.0.1' + expected = [ip_address] + pg = object_utils.create_test_portgroup(self.context, + node_id=self.node.id, + address='aa:bb:cc:dd:ee:ff', + uuid=uuidutils.generate_uuid(), + extra={'vif_port_id': + 'test-vif-A'}, + driver='fake') + mock_gfia.return_value = ip_address + with task_manager.acquire(self.context, self.node.uuid) as task: + api = dhcp_factory.DHCPFactory().provider + result = api._get_ip_addresses(task, [pg], mock.sentinel.client) + self.assertEqual(expected, result) @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_port_ip_address') def test_get_ip_addresses(self, get_ip_mock): @@ -359,10 +425,26 @@ class TestNeutron(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid) as task: api = dhcp_factory.DHCPFactory().provider result = api.get_ip_addresses(task) - get_ip_mock.assert_called_once_with(task, self.ports[0].uuid, + get_ip_mock.assert_called_once_with(task, task.ports[0], mock.ANY) self.assertEqual(expected, result) + @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi._get_port_ip_address') + def test_get_ip_addresses_for_port_and_portgroup(self, get_ip_mock): + object_utils.create_test_portgroup(self.context, + node_id=self.node.id, + address='aa:bb:cc:dd:ee:ff', + uuid=uuidutils.generate_uuid(), + extra={'vif_port_id': + 'test-vif-A'}, + driver='fake') + with task_manager.acquire(self.context, self.node.uuid) as task: + api = dhcp_factory.DHCPFactory().provider + api.get_ip_addresses(task) + get_ip_mock.assert_has_calls( + [mock.call(task, task.ports[0], mock.ANY), + mock.call(task, task.portgroups[0], mock.ANY)]) + @mock.patch.object(client.Client, 'create_port') def test_create_cleaning_ports(self, create_mock): # Ensure we can create cleaning ports for in band cleaning