objects: adding a parent_addr field to the PciDevice object

The parent_addr field will help identify the relationship between
Physical and Virtual pci device functions.

This information was already available to the PciDevice objects and was
stored in extra_info['phys_function'] field, so we add code that will
migrate old data on the fly, for live upgrade scenarios where we still
have running older compute nodes alongside new ones.

We don't want to migrate, however, if there are still any API service
instances running that might access this info directly (without the help
of the conductor) but have not been upgraded yet.

Co-authored-by: Nikola Đipanov <ndipanov@redhat.com>
Partially implements blueprint sriov-physical-function-passthrough
Change-Id: I94e8ce2c2a3d1c9e8b4aa1b245076eba84f37f45

nova/objects/pci_device.py

@@ -18,6 +18,7 @@
 from oslo_serialization import jsonutils
 from oslo_utils import versionutils
 
+from nova import context
 from nova import db
 from nova import exception
 from nova import objects
@@ -85,7 +86,8 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject):
     # Version 1.1: String attributes updated to support unicode
     # Version 1.2: added request_id field
     # Version 1.3: Added field to represent PCI device NUMA node
-    VERSION = '1.3'
+    # Version 1.4: Added parent_addr field
+    VERSION = '1.4'
 
     fields = {
         'id': fields.IntegerField(),
@@ -103,12 +105,30 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject):
         'request_id': fields.StringField(nullable=True),
         'extra_info': fields.DictOfStringsField(),
         'numa_node': fields.IntegerField(nullable=True),
+        'parent_addr': fields.StringField(nullable=True),
     }
 
+    @staticmethod
+    def _migrate_parent_addr():
+        # NOTE(ndipanov): Only migrate parent_addr if all services are up to at
+        # least version 4 - this should only ever be called from save()
+        services = ('conductor', 'api')
+        min_parent_addr_version = 4
+
+        min_deployed = min(objects.Service.get_minimum_version(
+            context.get_admin_context(), 'nova-' + service)
+            for service in services)
+        return min_deployed >= min_parent_addr_version
+
     def obj_make_compatible(self, primitive, target_version):
         target_version = versionutils.convert_version_to_tuple(target_version)
         if target_version < (1, 2) and 'request_id' in primitive:
             del primitive['request_id']
+        if target_version < (1, 4) and 'parent_addr' in primitive:
+            if primitive['parent_addr'] is not None:
+                extra_info = primitive.get('extra_info', {})
+                extra_info['phys_function'] = primitive['parent_addr']
+            del primitive['parent_addr']
 
     def update_device(self, dev_dict):
         """Sync the content from device dictionary to device object.
@@ -156,6 +176,11 @@ def _from_db_object(context, pci_device, db_dev):
                 pci_device.extra_info = jsonutils.loads(extra_info)
         pci_device._context = context
         pci_device.obj_reset_changes()
+        # NOTE(ndipanov): As long as there is PF data in the old location, we
+        # want to load it as it may have be the only place we have it
+        if 'phys_function' in pci_device.extra_info:
+            pci_device.parent_addr = pci_device.extra_info['phys_function']
+
         return pci_device
 
     @base.remotable_classmethod
@@ -189,6 +214,24 @@ def save(self):
                                   self.address)
         elif self.status != fields.PciDeviceStatus.DELETED:
             updates = self.obj_get_changes()
+            if not self._migrate_parent_addr():
+                # NOTE(ndipanov): If we are not migrating data yet, make sure
+                # that any changes to parent_addr are also in the old location
+                # in extra_info
+                if 'parent_addr' in updates:
+                    extra_update = updates.get('extra_info', {})
+                    if not extra_update and self.obj_attr_is_set('extra_info'):
+                        extra_update = self.extra_info
+                    extra_update['phys_function'] = updates['parent_addr']
+                    updates['extra_info'] = extra_update
+            else:
+                # NOTE(ndipanov): Once we start migrating, meaning all control
+                # plane has been upgraded - aggressively migrate on every save
+                pf_extra = self.extra_info.pop('phys_function', None)
+                if pf_extra and 'parent_addr' not in updates:
+                    updates['parent_addr'] = pf_extra
+                updates['extra_info'] = self.extra_info
+
             if 'extra_info' in updates:
                 updates['extra_info'] = jsonutils.dumps(updates['extra_info'])
             if updates:
@@ -270,14 +313,18 @@ def free(self, instance=None):
             else:
                 instance.pci_devices.objects.remove(existed)
 
+    def is_available(self):
+        return self.status == fields.PciDeviceStatus.AVAILABLE
+
 
 @base.NovaObjectRegistry.register
 class PciDeviceList(base.ObjectListBase, base.NovaObject):
     # Version 1.0: Initial version
     #              PciDevice <= 1.1
     # Version 1.1: PciDevice 1.2
     # Version 1.2: PciDevice 1.3
-    VERSION = '1.2'
+    # Version 1.3: Adds get_by_parent_address
+    VERSION = '1.3'
 
     fields = {
         'objects': fields.ListOfObjectsField('PciDevice'),
@@ -299,3 +346,11 @@ def get_by_instance_uuid(cls, context, uuid):
         db_dev_list = db.pci_device_get_all_by_instance_uuid(context, uuid)
         return base.obj_make_list(context, cls(context), objects.PciDevice,
                                   db_dev_list)
+
+    @base.remotable_classmethod
+    def get_by_parent_address(cls, context, node_id, parent_addr):
+        db_dev_list = db.pci_device_get_all_by_parent_addr(context,
+                                                           node_id,
+                                                           parent_addr)
+        return base.obj_make_list(context, cls(context), objects.PciDevice,
+                                  db_dev_list)

nova/objects/service.py

@@ -28,7 +28,7 @@
 
 
 # NOTE(danms): This is the global service version counter
-SERVICE_VERSION = 3
+SERVICE_VERSION = 4
 
 
 # NOTE(danms): This is our SERVICE_VERSION history. The idea is that any
@@ -58,6 +58,8 @@
     {'compute_rpc': '4.5'},
     # Version 3: Add trigger_crash_dump method to compute rpc api
     {'compute_rpc': '4.6'},
+    # Version 4: Add PciDevice.parent_addr (data migration needed)
+    {'compute_rpc': '4.6'},
 )
 
 

nova/pci/devspec.py

@@ -161,19 +161,15 @@ def match(self, dev_dict):
             self.vendor_id in (ANY, dev_dict['vendor_id']),
             self.product_id in (ANY, dev_dict['product_id']),
             self.address.match(dev_dict['address'],
-                dev_dict.get('phys_function'))
+                dev_dict.get('parent_addr'))
             ]
         return all(conditions)
 
     def match_pci_obj(self, pci_obj):
-        if pci_obj.extra_info:
-            phy_func = pci_obj.extra_info.get('phys_function')
-        else:
-            phy_func = None
         return self.match({'vendor_id': pci_obj.vendor_id,
                             'product_id': pci_obj.product_id,
                             'address': pci_obj.address,
-                            'phys_function': phy_func})
+                            'parent_addr': pci_obj.parent_addr})
 
     def get_tags(self):
         return self.tags

nova/tests/unit/objects/test_instance.py

@@ -764,6 +764,7 @@ def test_with_pci_devices(self):
              'label': 'l',
              'instance_uuid': fake_uuid,
              'request_id': None,
+             'parent_addr': None,
              'extra_info': '{}'},
             {
              'created_at': None,
@@ -782,6 +783,7 @@ def test_with_pci_devices(self):
              'label': 'l',
              'instance_uuid': fake_uuid,
              'request_id': None,
+             'parent_addr': 'a1',
              'extra_info': '{}'},
             ]
         self.mox.StubOutWithMock(db, 'instance_get_by_uuid')

nova/tests/unit/objects/test_objects.py

@@ -1169,8 +1169,8 @@ def obj_name(cls):
     'NetworkList': '1.2-69eca910d8fa035dfecd8ba10877ee59',
     'NetworkRequest': '1.1-7a3e4ca2ce1e7b62d8400488f2f2b756',
     'NetworkRequestList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e',
-    'PciDevice': '1.3-d92e0b17bbed61815b919af6b8d8998e',
-    'PciDeviceList': '1.2-3757458c45591cbc92c72ee99e757c98',
+    'PciDevice': '1.4-4f54e80054bbb6414e17eb9babc97a44',
+    'PciDeviceList': '1.3-52ff14355491c8c580bdc0ba34c26210',
     'PciDevicePool': '1.1-3f5ddc3ff7bfa14da7f6c7e9904cc000',
     'PciDevicePoolList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e',
     'Quotas': '1.2-1fe4cd50593aaf5d36a6dc5ab3f98fb3',

nova/tests/unit/objects/test_pci_device.py

@@ -15,11 +15,13 @@
 
 import copy
 
+import mock
 from oslo_utils import timeutils
 
 from nova import context
 from nova import db
 from nova import exception
+from nova import objects
 from nova.objects import fields
 from nova.objects import instance
 from nova.objects import pci_device
@@ -39,6 +41,7 @@
     'updated_at': None,
     'deleted_at': None,
     'deleted': None,
+    'parent_addr': None,
     'id': 1,
     'compute_node_id': 1,
     'address': 'a',
@@ -61,6 +64,7 @@
     'deleted_at': None,
     'deleted': None,
     'id': 2,
+    'parent_addr': 'a',
     'compute_node_id': 1,
     'address': 'a1',
     'vendor_id': 'v1',
@@ -76,6 +80,28 @@
     }
 
 
+fake_db_dev_old = {
+    'created_at': None,
+    'updated_at': None,
+    'deleted_at': None,
+    'deleted': None,
+    'id': 2,
+    'parent_addr': None,
+    'compute_node_id': 1,
+    'address': 'a1',
+    'vendor_id': 'v1',
+    'product_id': 'p1',
+    'numa_node': 1,
+    'dev_type': fields.PciDeviceType.SRIOV_VF,
+    'status': fields.PciDeviceStatus.AVAILABLE,
+    'dev_id': 'i',
+    'label': 'l',
+    'instance_uuid': None,
+    'extra_info': '{"phys_function": "blah"}',
+    'request_id': None,
+    }
+
+
 class _TestPciDeviceObject(object):
     def _create_fake_instance(self):
         self.inst = instance.Instance()
@@ -147,6 +173,13 @@ def test_get_by_dev_id(self):
         self.assertEqual(self.pci_device.product_id, 'p')
         self.assertEqual(self.pci_device.obj_what_changed(), set())
 
+    def test_from_db_obj_pre_1_4_format(self):
+        ctxt = context.get_admin_context()
+        dev = pci_device.PciDevice._from_db_object(
+            ctxt, pci_device.PciDevice(), fake_db_dev_old)
+        self.assertEqual('blah', dev.parent_addr)
+        self.assertEqual({'phys_function': 'blah'}, dev.extra_info)
+
     def test_save(self):
         ctxt = context.get_admin_context()
         self._create_fake_pci_device(ctxt=ctxt)
@@ -206,6 +239,49 @@ def _fake_update(ctxt, node_id, addr, updates):
         self.pci_device.save()
         self.assertEqual(self.called, False)
 
+    @mock.patch.object(objects.Service, 'get_minimum_version', return_value=4)
+    def test_save_migrate_parent_addr(self, get_min_ver_mock):
+        ctxt = context.get_admin_context()
+        dev = pci_device.PciDevice._from_db_object(
+            ctxt, pci_device.PciDevice(), fake_db_dev_old)
+        with mock.patch.object(db, 'pci_device_update',
+                               return_value=fake_db_dev_old) as update_mock:
+            dev.save()
+            update_mock.assert_called_once_with(
+                ctxt, dev.compute_node_id, dev.address,
+                {'extra_info': '{}', 'parent_addr': 'blah'})
+
+    @mock.patch.object(objects.Service, 'get_minimum_version', return_value=4)
+    def test_save_migrate_parent_addr_updated(self, get_min_ver_mock):
+        ctxt = context.get_admin_context()
+        dev = pci_device.PciDevice._from_db_object(
+            ctxt, pci_device.PciDevice(), fake_db_dev_old)
+        # Note that the pci manager code will never update parent_addr alone,
+        # but we want to make it future proof so we guard against it
+        dev.parent_addr = 'doh!'
+        with mock.patch.object(db, 'pci_device_update',
+                               return_value=fake_db_dev_old) as update_mock:
+            dev.save()
+            update_mock.assert_called_once_with(
+                ctxt, dev.compute_node_id, dev.address,
+                {'extra_info': '{}', 'parent_addr': 'doh!'})
+
+    @mock.patch.object(objects.Service, 'get_minimum_version', return_value=2)
+    def test_save_dont_migrate_parent_addr(self, get_min_ver_mock):
+        ctxt = context.get_admin_context()
+        dev = pci_device.PciDevice._from_db_object(
+            ctxt, pci_device.PciDevice(), fake_db_dev_old)
+        dev.extra_info['other'] = "blahtoo"
+        with mock.patch.object(db, 'pci_device_update',
+                               return_value=fake_db_dev_old) as update_mock:
+            dev.save()
+            self.assertEqual("blah",
+                             update_mock.call_args[0][3]['parent_addr'])
+            self.assertIn("phys_function",
+                          update_mock.call_args[0][3]['extra_info'])
+            self.assertIn("other",
+                          update_mock.call_args[0][3]['extra_info'])
+
     def test_update_numa_node(self):
         self.pci_device = pci_device.PciDevice.create(dev_dict)
         self.assertEqual(0, self.pci_device.numa_node)

nova/tests/unit/pci/test_devspec.py

@@ -23,7 +23,7 @@
 dev = {"vendor_id": "8086",
        "product_id": "5057",
        "address": "1234:5678:8988.5",
-       "phys_function": "0000:0a:00.0"}
+       "parent_addr": "0000:0a:00.0"}
 
 
 class PciAddressTestCase(test.NoDBTestCase):
@@ -92,7 +92,7 @@ def test_partial_address(self):
         dev = {"vendor_id": "1137",
                "product_id": "0071",
                "address": "0000:0a:00.5",
-               "phys_function": "0000:0a:00.0"}
+               "parent_addr": "0000:0a:00.0"}
         self.assertTrue(pci.match(dev))
 
     @mock.patch('nova.pci.utils.is_physical_function', return_value = True)
@@ -169,6 +169,7 @@ def test_pci_obj(self):
             'product_id': '5057',
             'vendor_id': '8086',
             'status': 'available',
+            'parent_addr': None,
             'extra_k1': 'v1',
         }
 

nova/tests/unit/pci/test_manager.py

@@ -60,13 +60,14 @@
     'instance_uuid': None,
     'extra_info': '{}',
     'request_id': None,
+    'parent_addr': None,
     }
 fake_db_dev_1 = dict(fake_db_dev, vendor_id='v1',
                      product_id='p1', id=2,
                      address='0000:00:00.2',
                      numa_node=0)
 fake_db_dev_2 = dict(fake_db_dev, id=3, address='0000:00:00.3',
-                     numa_node=None)
+                     numa_node=None, parent_addr='0000:00:00.1')
 fake_db_devs = [fake_db_dev, fake_db_dev_1, fake_db_dev_2]
 
 
@@ -272,7 +273,9 @@ def test_update_pci_for_migration_out(self, mock_get):
                               dev in self.tracker.pci_devs]),
                          set(['v', 'v1']))
 
-    def test_save(self):
+    @mock.patch.object(objects.PciDevice, '_migrate_parent_addr',
+                       return_value=False)
+    def test_save(self, migrate_mock):
         self.stub_out(
                 'nova.db.pci_device_update',
                 self._fake_pci_device_update)

nova/tests/unit/pci/test_stats.py

@@ -218,6 +218,8 @@ def _create_pci_devices(self):
                        'product_id': '0071',
                        'status': 'available',
                        'request_id': None,
+                       'dev_type': 'type-PCI',
+                       'parent_addr': None,
                        'numa_node': 0}
             self.pci_tagged_devices.append(objects.PciDevice.create(pci_dev))
 
@@ -229,6 +231,8 @@ def _create_pci_devices(self):
                        'product_id': '0072',
                        'status': 'available',
                        'request_id': None,
+                       'dev_type': 'type-PCI',
+                       'parent_addr': None,
                        'numa_node': 0}
             self.pci_untagged_devices.append(objects.PciDevice.create(pci_dev))
 
@@ -286,6 +290,7 @@ def test_add_device_no_devspec(self):
                    'vendor_id': '2345',
                    'product_id': '0172',
                    'status': 'available',
+                   'parent_addr': None,
                    'request_id': None}
         pci_dev_obj = objects.PciDevice.create(pci_dev)
         self.pci_stats.add_device(pci_dev_obj)
@@ -301,6 +306,7 @@ def test_remove_device_no_devspec(self):
                    'vendor_id': '2345',
                    'product_id': '0172',
                    'status': 'available',
+                   'parent_addr': None,
                    'request_id': None}
         pci_dev_obj = objects.PciDevice.create(pci_dev)
         self.pci_stats.remove_device(pci_dev_obj)