From 6f162c3422df6c11b0d9f548487bfb3b9e401ca5 Mon Sep 17 00:00:00 2001 From: Thomas Gao Date: Fri, 7 Feb 2020 15:28:42 -0500 Subject: [PATCH] Fixed address interface foreign key inconsistency Foreign key in sysinv.object.address.Address is `interface_uuid`, which is inconsistent with the foreign key `interface_id` defined in the database schema. This fix corrected that. Added a unit test to verify that addresses associated with an interface could be deleted. Additionally wrote a set of TODO unit tests blocked by the bug: tested delete address for orphaned-routes case, unlocked host state, and the case where address is allocated from pool. Modified interface querying mechanism to look up all interfaces. This modification is necessary because the current implementation of add_interface_filter only looks up those of type ethernet, ae and vlan. Attempting to get an virtual-type interface will raise an exception, causing Jenkins installation to fail. After a visual inspection of interface_uuid occurrences, fixed a few other occurrences of bad address.interface_uuid that are not caught by the unit test. Added new unit test suites in place to cover the code paths. Closes-Bug: 1861131 Change-Id: I6f2449bbbb69d6f2353e521bfcd138d880ce878f Signed-off-by: Thomas Gao --- .../sysinv/api/controllers/v1/address.py | 13 +- .../sysinv/sysinv/sysinv/conductor/manager.py | 9 +- .../sysinv/sysinv/sysinv/db/sqlalchemy/api.py | 12 +- sysinv/sysinv/sysinv/sysinv/helm/nova.py | 2 +- .../sysinv/sysinv/sysinv/objects/address.py | 4 +- .../sysinv/sysinv/tests/api/test_address.py | 119 +++++++++++++++++- .../sysinv/tests/conductor/test_manager.py | 56 +++++++++ .../sysinv/sysinv/tests/helm/test_nova.py | 28 +++++ 8 files changed, 219 insertions(+), 24 deletions(-) create mode 100644 sysinv/sysinv/sysinv/sysinv/tests/helm/test_nova.py diff --git a/sysinv/sysinv/sysinv/sysinv/api/controllers/v1/address.py b/sysinv/sysinv/sysinv/sysinv/api/controllers/v1/address.py index 910379e6f4..7e43764d56 100644 --- a/sysinv/sysinv/sysinv/sysinv/api/controllers/v1/address.py +++ b/sysinv/sysinv/sysinv/sysinv/api/controllers/v1/address.py @@ -87,7 +87,13 @@ class Address(base.APIBase): "The UUID of the address pool from which this address was allocated" def __init__(self, **kwargs): - self.fields = objects.address.fields.keys() + # The interface_uuid in this `Address` type is kept to avoid changes to + # API/CLI. However, `self.field` refers to `objects.address.field` which + # doesn't include 'interface_uuid', and therefore it is added manually. + # Otherwise, controller `Address.as_dict()` will not include `interface_uuid` + # despite the field being present. + self.fields = list(objects.address.fields.keys()) + self.fields.append('interface_uuid') for k in self.fields: if not hasattr(self, k): # Skip fields that we choose to hide @@ -110,6 +116,9 @@ class Address(base.APIBase): @classmethod def convert_with_links(cls, rpc_address, expand=True): address = Address(**rpc_address.as_dict()) + if rpc_address.interface_id: + address.interface_uuid = pecan.request.dbapi.iinterface_get( + rpc_address.interface_id).uuid if not expand: address.unset_fields_except(['uuid', 'address', 'prefix', 'interface_uuid', 'ifname', @@ -285,7 +294,7 @@ class AddressController(rest.RestController): raise exception.AddressInSameSubnetExists( **{'address': entry['address'], 'prefix': entry['prefix'], - 'interface': entry['interface_uuid']}) + 'interface': entry['interface_id']}) def _check_address_count(self, interface_id, host_id): interface = pecan.request.dbapi.iinterface_get(interface_id) diff --git a/sysinv/sysinv/sysinv/sysinv/conductor/manager.py b/sysinv/sysinv/sysinv/sysinv/conductor/manager.py index c0b0476383..7afacabb3d 100644 --- a/sysinv/sysinv/sysinv/sysinv/conductor/manager.py +++ b/sysinv/sysinv/sysinv/sysinv/conductor/manager.py @@ -1245,11 +1245,11 @@ class ConductorManager(service.PeriodicService): return address = self.dbapi.address_get_by_name(address_name) - interface_uuid = address.interface_uuid + interface_id = address.interface_id ip_address = address.address - if interface_uuid: - interface = self.dbapi.iinterface_get(interface_uuid) + if interface_id: + interface = self.dbapi.iinterface_get(interface_id) mac_address = interface.imac elif network_type == constants.NETWORK_TYPE_MGMT: ihost = self.dbapi.ihost_get_by_hostname(hostname) @@ -1797,7 +1797,6 @@ class ConductorManager(service.PeriodicService): :param inic_dict_array: initial values for iport objects :returns: pass or fail """ - LOG.debug("Entering iport_update_by_ihost %s %s" % (ihost_uuid, inic_dict_array)) ihost_uuid.strip() @@ -2079,7 +2078,7 @@ class ConductorManager(service.PeriodicService): addr_name = cutils.format_address_name(ihost.hostname, networktype) address = self.dbapi.address_get_by_name(addr_name) - if address['interface_uuid'] is None: + if address['interface_id'] is None: self.dbapi.address_update(address['uuid'], values) except exception.AddressNotFoundByName: pass diff --git a/sysinv/sysinv/sysinv/sysinv/db/sqlalchemy/api.py b/sysinv/sysinv/sysinv/sysinv/db/sqlalchemy/api.py index f0c2fc494a..c8dfce0c40 100644 --- a/sysinv/sysinv/sysinv/sysinv/db/sqlalchemy/api.py +++ b/sysinv/sysinv/sysinv/sysinv/db/sqlalchemy/api.py @@ -347,17 +347,11 @@ def add_interface_filter(query, value): :return: Modified query. """ if utils.is_valid_mac(value): - return query.filter(or_(models.EthernetInterfaces.imac == value, - models.AeInterfaces.imac == value, - models.VlanInterfaces.imac == value)) + return query.filter(models.Interfaces.imac == value) elif uuidutils.is_uuid_like(value): - return query.filter(or_(models.EthernetInterfaces.uuid == value, - models.AeInterfaces.uuid == value, - models.VlanInterfaces.uuid == value)) + return query.filter(models.Interfaces.uuid == value) elif utils.is_int_like(value): - return query.filter(or_(models.EthernetInterfaces.id == value, - models.AeInterfaces.id == value, - models.VlanInterfaces.id == value)) + return query.filter(models.Interfaces.id == value) else: return add_identity_filter(query, value, use_ifname=True) diff --git a/sysinv/sysinv/sysinv/sysinv/helm/nova.py b/sysinv/sysinv/sysinv/sysinv/helm/nova.py index 8c2f2616c2..ba4fd51c48 100644 --- a/sysinv/sysinv/sysinv/sysinv/helm/nova.py +++ b/sysinv/sysinv/sysinv/sysinv/helm/nova.py @@ -388,7 +388,7 @@ class NovaHelm(openstack.OpenstackBaseHelm): cluster_host_ip = None ip_family = None for addr in addresses: - if addr.interface_uuid == cluster_host_iface.uuid: + if addr.interface_id == cluster_host_iface.id: cluster_host_ip = addr.address ip_family = addr.family diff --git a/sysinv/sysinv/sysinv/sysinv/objects/address.py b/sysinv/sysinv/sysinv/sysinv/objects/address.py index 488e3be217..10b77ee6e6 100644 --- a/sysinv/sysinv/sysinv/sysinv/objects/address.py +++ b/sysinv/sysinv/sysinv/sysinv/objects/address.py @@ -22,7 +22,7 @@ class Address(base.SysinvObject): fields = {'id': int, 'uuid': utils.uuid_or_none, 'forihostid': utils.int_or_none, - 'interface_uuid': utils.uuid_or_none, + 'interface_id': utils.int_or_none, 'pool_uuid': utils.uuid_or_none, 'ifname': utils.str_or_none, 'family': utils.int_or_none, @@ -32,7 +32,7 @@ class Address(base.SysinvObject): 'name': utils.str_or_none, } - _foreign_fields = {'interface_uuid': 'interface:uuid', + _foreign_fields = {'interface_id': 'interface:id', 'pool_uuid': 'address_pool:uuid', 'ifname': 'interface:ifname', 'forihostid': 'interface:forihostid'} diff --git a/sysinv/sysinv/sysinv/sysinv/tests/api/test_address.py b/sysinv/sysinv/sysinv/sysinv/tests/api/test_address.py index 845db24ce0..24fbc1babc 100644 --- a/sysinv/sysinv/sysinv/sysinv/tests/api/test_address.py +++ b/sysinv/sysinv/sysinv/sysinv/tests/api/test_address.py @@ -8,6 +8,7 @@ Tests for the API / address / methods. """ +import mock import netaddr from six.moves import http_client @@ -248,6 +249,8 @@ class TestDelete(AddressTestCase): def setUp(self): super(TestDelete, self).setUp() + self.worker = self._create_test_host(constants.WORKER, + administrative=constants.ADMIN_LOCKED) def test_delete(self): # Delete the API object @@ -259,11 +262,117 @@ class TestDelete(AddressTestCase): # Verify the expected API response for the delete self.assertEqual(response.status_code, http_client.NO_CONTENT) - # TODO: Add unit tests to verify deletion is rejected as expected by - # _check_orphaned_routes, _check_host_state, and _check_from_pool. - # - # Currently blocked by bug in dbapi preventing testcase setup: - # https://bugs.launchpad.net/starlingx/+bug/1861131 + def test_delete_address_with_interface(self): + interface = dbutils.create_test_interface( + ifname="test0", + ifclass=constants.INTERFACE_CLASS_PLATFORM, + forihostid=self.worker.id, + ihost_uuid=self.worker.uuid) + + address = dbutils.create_test_address( + interface_id=interface.id, + name="enptest01", + family=self.oam_subnet.version, + address=str(self.oam_subnet[25]), + prefix=self.oam_subnet.prefixlen) + self.assertEqual(address["interface_id"], interface.id) + + response = self.delete(self.get_single_url(address.uuid), + headers=self.API_HEADERS) + self.assertEqual(response.status_code, http_client.NO_CONTENT) + + def test_orphaned_routes(self): + interface = dbutils.create_test_interface( + ifname="test0", + ifclass=constants.INTERFACE_CLASS_PLATFORM, + forihostid=self.worker.id, + ihost_uuid=self.worker.uuid) + + address = dbutils.create_test_address( + interface_id=interface.id, + name="enptest01", + family=self.oam_subnet.version, + address=str(self.oam_subnet[25]), + prefix=self.oam_subnet.prefixlen) + self.assertEqual(address["interface_id"], interface.id) + + route = dbutils.create_test_route( + interface_id=interface.id, + family=4, + network='10.10.10.0', + prefix=24, + gateway=str(self.oam_subnet[1]), + ) + self.assertEqual(route['gateway'], str(self.oam_subnet[1])) + + response = self.delete(self.get_single_url(address.uuid), + headers=self.API_HEADERS, + expect_errors=True) + self.assertEqual(response.content_type, 'application/json') + self.assertEqual(response.status_code, http_client.CONFLICT) + self.assertIn( + "Address %s is in use by a route to %s/%d via %s" % ( + address["address"], route["network"], route["prefix"], + route["gateway"] + ), response.json['error_message']) + + def test_bad_host_state(self): + interface = dbutils.create_test_interface( + ifname="test0", + ifclass=constants.INTERFACE_CLASS_PLATFORM, + forihostid=self.worker.id, + ihost_uuid=self.worker.uuid) + + address = dbutils.create_test_address( + interface_id=interface.id, + name="enptest01", + family=self.oam_subnet.version, + address=str(self.oam_subnet[25]), + prefix=self.oam_subnet.prefixlen) + self.assertEqual(address["interface_id"], interface.id) + + # unlock the worker + dbapi = dbutils.db_api.get_instance() + worker = dbapi.ihost_update(self.worker.uuid, { + "administrative": constants.ADMIN_UNLOCKED + }) + self.assertEqual(worker['administrative'], + constants.ADMIN_UNLOCKED) + + response = self.delete(self.get_single_url(address.uuid), + headers=self.API_HEADERS, + expect_errors=True) + self.assertEqual(response.content_type, 'application/json') + self.assertEqual(response.status_code, + http_client.INTERNAL_SERVER_ERROR) + self.assertIn("administrative state = unlocked", + response.json['error_message']) + + def test_delete_address_from_pool(self): + pool = dbutils.create_test_address_pool( + name='testpool', + network='192.168.204.0', + ranges=[['192.168.204.2', '192.168.204.254']], + prefix=24) + address = dbutils.create_test_address( + name="enptest01", + family=4, + address='192.168.204.4', + prefix=24, + address_pool_id=pool.id) + self.assertEqual(address['pool_uuid'], pool.uuid) + + with mock.patch( + 'sysinv.common.utils.is_initial_config_complete', lambda: True): + response = self.delete(self.get_single_url(address.uuid), + headers=self.API_HEADERS, + expect_errors=True) + self.assertEqual(response.content_type, 'application/json') + self.assertEqual(response.status_code, + http_client.CONFLICT) + self.assertIn("Address has been allocated from pool; " + "cannot be manually deleted", + response.json['error_message']) class TestList(AddressTestCase): diff --git a/sysinv/sysinv/sysinv/sysinv/tests/conductor/test_manager.py b/sysinv/sysinv/sysinv/sysinv/tests/conductor/test_manager.py index a086698c9f..32263d547f 100644 --- a/sysinv/sysinv/sysinv/sysinv/tests/conductor/test_manager.py +++ b/sysinv/sysinv/sysinv/sysinv/tests/conductor/test_manager.py @@ -1426,3 +1426,59 @@ class ManagerTestCase(base.DbTestCase): updated_port = self.dbapi.ethernet_port_get(port1['uuid'], host_id) self.assertEqual(updated_port['node_id'], 3) + + +class ManagerTestCaseInternal(base.BaseHostTestCase): + + def setUp(self): + super(ManagerTestCaseInternal, self).setUp() + + # Set up objects for testing + self.service = manager.ConductorManager('test-host', 'test-topic') + self.service.dbapi = dbapi.get_instance() + + def test_remove_lease_for_address(self): + # create test interface + ihost = self._create_test_host( + personality=constants.WORKER, + administrative=constants.ADMIN_UNLOCKED) + iface = utils.create_test_interface( + ifname="test0", + ifclass=constants.INTERFACE_CLASS_PLATFORM, + forihostid=ihost.id, + ihost_uuid=ihost.uuid) + network = self.dbapi.network_get_by_type(constants.NETWORK_TYPE_MGMT) + utils.create_test_interface_network( + interface_id=iface.id, + network_id=network.id) + + # create test address associated with interface + address_name = cutils.format_address_name(ihost.hostname, + network.type) + self.dbapi.address_create({ + 'name': address_name, + 'family': self.oam_subnet.version, + 'prefix': self.oam_subnet.prefixlen, + 'address': str(self.oam_subnet[24]), + 'interface_id': iface.id, + 'enable_dad': self.oam_subnet.version == 6 + }) + + # stub the system i/o calls + self.mock_objs = [ + mock.patch.object( + manager.ConductorManager, '_find_local_interface_name', + lambda x, y: iface.ifname), + mock.patch('sysinv.common.utils.get_dhcp_cid', + lambda x, y, z: None), + mock.patch.object( + manager.ConductorManager, '_dhcp_release', + lambda a, b, c, d, e: None) + ] + + for mock_obj in self.mock_objs: + mock_obj.start() + self.addCleanup(mock_obj.stop) + + self.service._remove_lease_for_address(ihost.hostname, + constants.NETWORK_TYPE_MGMT) diff --git a/sysinv/sysinv/sysinv/sysinv/tests/helm/test_nova.py b/sysinv/sysinv/sysinv/sysinv/tests/helm/test_nova.py new file mode 100644 index 0000000000..35df2d98c5 --- /dev/null +++ b/sysinv/sysinv/sysinv/sysinv/tests/helm/test_nova.py @@ -0,0 +1,28 @@ +from sysinv.helm import nova +from sysinv.helm import helm +from sysinv.common import constants + +from sysinv.tests.db import base as dbbase + + +class NovaGetOverrideTest(dbbase.ControllerHostTestCase): + + def setUp(self): + super(NovaGetOverrideTest, self).setUp() + self.operator = helm.HelmOperator(self.dbapi) + self.nova = nova.NovaHelm(self.operator) + self.worker = self._create_test_host( + personality=constants.WORKER, + administrative=constants.ADMIN_LOCKED) + self.ifaces = self._create_test_host_platform_interface(self.worker) + self.dbapi.address_create({ + 'name': 'test', + 'family': self.oam_subnet.version, + 'prefix': self.oam_subnet.prefixlen, + 'address': str(self.oam_subnet[24]), + 'interface_id': self.ifaces[0].id, + 'enable_dad': self.oam_subnet.version == 6 + }) + + def test_update_host_addresses(self): + self.nova._update_host_addresses(self.worker, {}, {}, {})