Optimize trait creation to check existence first

Comparing benchmarks of creating (repeated) resource classes with
traits, it became clear that creating resource classes [1] was about
1/3rd faster. Comparing the code, they were very similar, however
the resource class side was ordered to check for existence first
before attempting to do a create(). This meant that in the fairly common
case where a resource class already existed we could finish early.

This change updates the code so that the common idiom used in clients
of "ensuring a trait" with PUT /traits/$name routine to be the same
as the "ensuring a resource class" with PUT /resource_classes/$name
routine: check that it is already there, first. Since it is best
practice to always ensure trait or resource class before using a
CUSTOM_ one, this is a good orientation.

Note that with the recent addition of the AttributeCache, these queries
for get_by_name [2] go to the ResourceClass or Trait cache. However, at
this stage in processing, the cache is empty and calls to get_by_name
will fill it.

A unit test is added to confirm that the 'put_trait' handler will look
at get_by_name and then go back to assuming the trait exists if the
create() fails because it already exists. The general behavior of
whether a trait is created or updated is confirmed by the gabbit
tests in trait.yaml. That test also confirms that we have a legit
last-modified time when we fall through both exceptions and make
the assumption that a trait was created by some other thread. It is
safe to use 'now' as the last-modified time because it was created
in this same second, just not by us.

[1] 2f56f379a5/placement/handlers/resource_class.py (L209)
[2] 2f56f379a5/placement/objects/resource_class.py (L72)

Change-Id: I88e42ef18bd5b2616a93730241d2419c6b431276

placement/handlers/trait.py

@@ -78,22 +78,27 @@ def put_trait(req):
             '255 characters, start with the prefix "CUSTOM_" and use '
             'following characters: "A"-"Z", "0"-"9" and "_"')
 
-    trait = trait_obj.Trait(context)
-    trait.name = name
-
+    status = 204
     try:
-        trait.create()
-        req.response.status = 201
-    except exception.TraitExists:
-        # Get the trait that already exists to get last-modified time.
-        if want_version.matches((1, 15)):
-            trait = trait_obj.Trait.get_by_name(context, name)
-        req.response.status = 204
+        trait = trait_obj.Trait.get_by_name(context, name)
+    except exception.TraitNotFound:
+        try:
+            trait = trait_obj.Trait(context, name=name)
+            trait.create()
+            status = 201
+        except exception.TraitExists:
+            # Something just created the trait
+            pass
 
+    req.response.status = status
     req.response.content_type = None
     req.response.location = util.trait_url(req.environ, trait)
     if want_version.matches((1, 15)):
-        req.response.last_modified = trait.created_at
+        # If the TraitExists exception was hit above, created_at is None
+        # so fall back to now for the last modified header.
+        last_modified = (trait.created_at
+                         or timeutils.utcnow(with_timezone=True))
+        req.response.last_modified = last_modified
         req.response.cache_control = 'no-cache'
     return req.response
 

placement/tests/unit/handlers/test_trait.py

@@ -0,0 +1,69 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+"""Unit tests for code in the trait handler that gabbi cannot easily cover."""
+
+
+import microversion_parse
+import mock
+import webob
+
+from placement import context
+from placement import exception
+from placement.handlers import trait
+from placement.tests.unit import base
+
+
+class TestTraitHandler(base.ContextTestCase):
+
+    @mock.patch('placement.objects.trait.Trait.create')
+    @mock.patch('placement.objects.trait.Trait.get_by_name')
+    @mock.patch('placement.context.RequestContext.can')
+    @mock.patch('placement.util.wsgi_path_item', return_value='CUSTOM_FOOBAR')
+    def test_trait_create_ordering(
+            self, mock_path, mock_can, mock_get_by_name, mock_create):
+        """Test that we call Trait.create when get_by_name has a TraitNotFound
+        and that if create can't create, we assume 204.
+        """
+        # The trait doesn't initially exist.
+        mock_get_by_name.side_effect = exception.TraitNotFound(
+            name='CUSTOM_FOOBAR')
+        # But we fake that it does after first not finding it.
+        mock_create.side_effect = exception.TraitExists(
+            name='CUSTOM_FOOBAR')
+        fake_context = context.RequestContext(
+            user_id='fake', project_id='fake')
+
+        req = webob.Request.blank('/traits/CUSTOM_FOOBAR')
+        req.environ['placement.context'] = fake_context
+
+        parse_version = microversion_parse.parse_version_string
+        microversion = parse_version('1.15')
+        microversion.max_version = parse_version('9.99')
+        microversion.min_version = parse_version('1.0')
+        req.environ['placement.microversion'] = microversion
+
+        response = req.get_response(trait.put_trait)
+
+        # Trait was assumed to exist.
+        self.assertEqual('204 No Content', response.status)
+
+        # We get a last modified header, even though we don't know the exact
+        # create_at time (it is None on the Trait object and we fall back to
+        # now)
+        self.assertIn('last-modified', response.headers)
+
+        # Confirm we checked to see if the trait exists, but the
+        # side_effect happens
+        mock_get_by_name.assert_called_once_with(fake_context, 'CUSTOM_FOOBAR')
+
+        # Confirm we attempt to create the trait.
+        mock_create.assert_called_once_with()