Sync os-traits to Traits database table

When a new version of the os-traits library is released, the Traits
table in the api database needs to be updated to reflect those new
traits. This change does that, once, the first time either
Trait.get_by_name or TraitList.get_all is called in any process that is
using those objects.

This is an alternative to I729e84ea0ff8f333a156b24b15fe4d368209d015.
The major difference here is that there is no trait cache so the
surface area of the change is much smaller.

The list traits test in gabbit/traits.yaml has been changed to not
assert the length of traits returned: this can now change with each
release of the os-traits library.

Depends-on: I7a3f4bb8501fc3edad43e1aae5cb6b9ef1c0b00d
Co-Authored-By: Jay Pipes <jaypipes@gmail.com>
Change-Id: Ia92a4fd20b8991c6f4b34e3546cfb22aa5ed78aa

nova/objects/resource_provider.py

@@ -15,6 +15,8 @@ import copy
 # used over RPC. Remote manipulation is done with the placement HTTP
 # API. The 'remotable' decorators should not be used.
 
+import os_traits
+from oslo_concurrency import lockutils
 from oslo_db import exception as db_exc
 from oslo_log import log as logging
 from oslo_utils import versionutils
@@ -43,6 +45,8 @@ _AGG_TBL = models.PlacementAggregate.__table__
 _RP_AGG_TBL = models.ResourceProviderAggregate.__table__
 _RP_TRAIT_TBL = models.ResourceProviderTrait.__table__
 _RC_CACHE = None
+_TRAIT_LOCK = 'trait_sync'
+_TRAITS_SYNCED = False
 
 LOG = logging.getLogger(__name__)
 
@@ -61,6 +65,70 @@ def _ensure_rc_cache(ctx):
     _RC_CACHE = rc_cache.ResourceClassCache(ctx)
 
 
+@db_api.api_context_manager.writer
+def _trait_sync(ctx):
+    """Sync the os_traits symbols to the database.
+
+    Reads all symbols from the os_traits library, checks if any of them do
+    not exist in the database and bulk-inserts those that are not. This is
+    done once per process using this code if either Trait.get_by_name or
+    TraitList.get_all is called.
+
+    :param ctx: `nova.context.RequestContext` that may be used to grab a DB
+                connection.
+    """
+    # Create a set of all traits in the os_traits library.
+    std_traits = set(os_traits.get_traits())
+    conn = ctx.session.connection()
+    sel = sa.select([_TRAIT_TBL.c.name])
+    res = conn.execute(sel).fetchall()
+    # Create a set of all traits in the db that are not custom
+    # traits.
+    db_traits = set(
+        r[0] for r in res
+        if not os_traits.is_custom(r[0])
+    )
+    # Determine those traits which are in os_traits but not
+    # currently in the database, and insert them.
+    need_sync = std_traits - db_traits
+    ins = _TRAIT_TBL.insert()
+    batch_args = [
+        {'name': six.text_type(trait)}
+        for trait in need_sync
+    ]
+    if batch_args:
+        try:
+            conn.execute(ins, batch_args)
+            LOG.info("Synced traits from os_traits into API DB: %s",
+                     need_sync)
+        except db_exc.DBDuplicateEntry:
+            pass  # some other process sync'd, just ignore
+
+
+def _ensure_trait_sync(ctx):
+    """Ensures that the os_traits library is synchronized to the traits db.
+
+    If _TRAITS_SYNCED is False then this process has not tried to update the
+    traits db. Do so by calling _trait_sync. Since the placement API server
+    could be multi-threaded, lock around testing _TRAITS_SYNCED to avoid
+    duplicating work.
+
+    Different placement API server processes that talk to the same database
+    will avoid issues through the power of transactions.
+
+    :param ctx: `nova.context.RequestContext` that may be used to grab a DB
+                connection.
+    """
+    global _TRAITS_SYNCED
+    # If another thread is doing this work, wait for it to complete.
+    # When that thread is done _TRAITS_SYNCED will be true in this
+    # thread and we'll simply return.
+    with lockutils.lock(_TRAIT_LOCK):
+        if not _TRAITS_SYNCED:
+            _trait_sync(ctx)
+            _TRAITS_SYNCED = True
+
+
 def _get_current_inventory_resources(conn, rp):
     """Returns a set() containing the resource class IDs for all resources
     currently having an inventory record for the supplied resource provider.
@@ -2020,8 +2088,9 @@ class Trait(base.NovaObject):
         self._from_db_object(self._context, self, db_trait)
 
     @staticmethod
-    @db_api.api_context_manager.reader
+    @db_api.api_context_manager.writer  # trait sync can cause a write
     def _get_by_name_from_db(context, name):
+        _ensure_trait_sync(context)
         result = context.session.query(models.Trait).filter_by(
             name=name).first()
         if not result:
@@ -2071,8 +2140,9 @@ class TraitList(base.ObjectListBase, base.NovaObject):
     }
 
     @staticmethod
-    @db_api.api_context_manager.reader
+    @db_api.api_context_manager.writer  # trait sync can cause a write
     def _get_all_from_db(context, filters):
+        _ensure_trait_sync(context)
         if not filters:
             filters = {}
 

nova/tests/functional/api/openstack/placement/gabbits/traits.yaml

@@ -71,14 +71,17 @@ tests:
   response_forbidden_headers:
     - content-type
 
+# NOTE(cdent): This simply tests that traits we know should be
+# present are in the results. We can't check length here because
+# the standard traits, which will grow over time, are present.
 - name: list traits
   GET: /traits
   status: 200
-  response_json_paths:
-    $.traits.`len`: 2
   response_strings:
     - CUSTOM_TRAIT_1
     - CUSTOM_TRAIT_2
+    - MISC_SHARES_VIA_AGGREGATE
+    - HW_CPU_X86_SHA
 
 - name: list traits with invalid format of name parameter
   GET: /traits?name=in_abc

nova/tests/functional/db/test_resource_provider.py

@@ -12,7 +12,9 @@
 
 
 import mock
+import os_traits
 from oslo_db import exception as db_exc
+import sqlalchemy as sa
 
 import nova
 from nova import context
@@ -1534,6 +1536,11 @@ class ResourceClassTestCase(ResourceProviderBaseCase):
 
 class ResourceProviderTraitTestCase(ResourceProviderBaseCase):
 
+    def tearDown(self):
+        """Reset the _TRAITS_SYNCED boolean so it doesn't interfere."""
+        super(ResourceProviderTraitTestCase, self).tearDown()
+        rp_obj._TRAITS_SYNCED = False
+
     def _assert_traits(self, expected_traits, traits_objs):
         expected_traits.sort()
         traits = []
@@ -1542,6 +1549,11 @@ class ResourceProviderTraitTestCase(ResourceProviderBaseCase):
         traits.sort()
         self.assertEqual(expected_traits, traits)
 
+    def _assert_traits_in(self, expected_traits, traits_objs):
+        traits = [trait.name for trait in traits_objs]
+        for expected in expected_traits:
+            self.assertIn(expected, traits)
+
     def test_trait_create(self):
         t = objects.Trait(self.context)
         t.name = 'CUSTOM_TRAIT_A'
@@ -1603,8 +1615,8 @@ class ResourceProviderTraitTestCase(ResourceProviderBaseCase):
             t.name = name
             t.create()
 
-        self._assert_traits(trait_names,
-                            objects.TraitList.get_all(self.context))
+        self._assert_traits_in(trait_names,
+                               objects.TraitList.get_all(self.context))
 
     def test_traits_get_all_with_name_in_filter(self):
         trait_names = ['CUSTOM_TRAIT_A', 'CUSTOM_TRAIT_B', 'CUSTOM_TRAIT_C']
@@ -1734,10 +1746,29 @@ class ResourceProviderTraitTestCase(ResourceProviderBaseCase):
             filters={'name_in': ['CUSTOM_TRAIT_A', 'CUSTOM_TRAIT_B']})
         rp1.set_traits(associated_traits)
         rp2.set_traits(associated_traits)
-        self._assert_traits(['CUSTOM_TRAIT_C'],
+        self._assert_traits_in(['CUSTOM_TRAIT_C'],
             objects.TraitList.get_all(self.context,
                 filters={'associated': False}))
 
+    def test_sync_standard_traits(self):
+        """Tests that on a clean DB, we have zero traits in the DB but after
+        list all traits, os_traits have been synchronized.
+        """
+        std_traits = os_traits.get_traits()
+        conn = self.api_db.get_engine().connect()
+
+        def _db_traits(conn):
+            sel = sa.select([rp_obj._TRAIT_TBL.c.name])
+            return [r[0] for r in conn.execute(sel).fetchall()]
+
+        self.assertEqual([], _db_traits(conn))
+
+        all_traits = [trait.name for trait in
+                      objects.TraitList.get_all(self.context)]
+        self.assertEqual(set(std_traits), set(all_traits))
+        # confirm with a raw request
+        self.assertEqual(set(std_traits), set(_db_traits(conn)))
+
 
 class SharedProviderTestCase(ResourceProviderBaseCase):
     """Tests that the queries used to determine placement in deployments with

requirements.txt

@@ -53,6 +53,7 @@ oslo.middleware>=3.27.0 # Apache-2.0
 psutil>=3.2.2 # BSD
 oslo.versionedobjects>=1.17.0 # Apache-2.0
 os-brick>=1.13.1 # Apache-2.0
+os-traits>=0.3.1 # Apache-2.0
 os-vif>=1.4.0 # Apache-2.0
 os-win>=2.0.0 # Apache-2.0
 castellan>=0.7.0 # Apache-2.0