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 <Thomas.Gao@windriver.com>

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)

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:
+            if interface_id:
-                interface = self.dbapi.iinterface_get(interface_uuid)
+                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

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,
+        return query.filter(models.Interfaces.imac == value)
-                                models.AeInterfaces.imac == value,
-                                models.VlanInterfaces.imac == value))
     elif uuidutils.is_uuid_like(value):
-        return query.filter(or_(models.EthernetInterfaces.uuid == value,
+        return query.filter(models.Interfaces.uuid == value)
-                                models.AeInterfaces.uuid == value,
-                                models.VlanInterfaces.uuid == value))
     elif utils.is_int_like(value):
-        return query.filter(or_(models.EthernetInterfaces.id == value,
+        return query.filter(models.Interfaces.id == value)
-                                models.AeInterfaces.id == value,
-                                models.VlanInterfaces.id == value))
     else:
         return add_identity_filter(query, value, use_ifname=True)
 

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
 

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'}

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
+    def test_delete_address_with_interface(self):
-    # _check_orphaned_routes, _check_host_state, and _check_from_pool.
+        interface = dbutils.create_test_interface(
-    #
+            ifname="test0",
-    # Currently blocked by bug in dbapi preventing testcase setup:
+            ifclass=constants.INTERFACE_CLASS_PLATFORM,
-    #   https://bugs.launchpad.net/starlingx/+bug/1861131
+            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):

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)

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, {}, {}, {})