Optimize cluster list api

Up till now, cluster api controller cluster_template_id was a
property field loading the id from the DB every time. With this
change the field becomes of text type and mandatory, so wsme fwk
guarantees that the field is provided when needed.

Cluster objects will not load the cluster template on creation.
Instead cluster templates will be loaded when they are actually
needed.

story: 2006693
task: 36989

Co-Authored-By: Stavros Moiras <[email protected]>

Change-Id: I2313c6a8b647e521cfa476f9cec65ab286fa5a23

Author: Stavros Moiras

magnum/api/controllers/v1/cluster.py

@@ -68,25 +68,6 @@ class Cluster(base.APIBase):
     between the internal object model and the API representation of a Cluster.
     """
 
-    _cluster_template_id = None
-
-    def _get_cluster_template_id(self):
-        return self._cluster_template_id
-
-    def _set_cluster_template_id(self, value):
-        if value and self._cluster_template_id != value:
-            try:
-                cluster_template = api_utils.get_resource('ClusterTemplate',
-                                                          value)
-                self._cluster_template_id = cluster_template.uuid
-            except exception.ClusterTemplateNotFound as e:
-                # Change error code because 404 (NotFound) is inappropriate
-                # response for a POST request to create a Cluster
-                e.code = 400  # BadRequest
-                raise
-        elif value == wtypes.Unset:
-            self._cluster_template_id = wtypes.Unset
-
     uuid = types.uuid
     """Unique UUID for this cluster"""
 
@@ -95,10 +76,7 @@ def _set_cluster_template_id(self, value):
     """Name of this cluster, max length is limited to 242 because of heat
      stack requires max length limit to 255, and Magnum amend a uuid length"""
 
-    cluster_template_id = wsme.wsproperty(wtypes.text,
-                                          _get_cluster_template_id,
-                                          _set_cluster_template_id,
-                                          mandatory=True)
+    cluster_template_id = wsme.wsattr(wtypes.text, mandatory=True)
     """The cluster_template UUID"""
 
     keypair = wsme.wsattr(wtypes.StringType(min_length=1, max_length=255),
@@ -493,6 +471,7 @@ def _check_cluster_quota_limit(self, context):
 
     @base.Controller.api_version("1.1", "1.9")
     @expose.expose(ClusterID, body=Cluster, status_code=202)
+    @validation.ct_not_found_to_bad_request()
     @validation.enforce_cluster_type_supported()
     @validation.enforce_cluster_volume_storage_size()
     def post(self, cluster):
@@ -519,8 +498,10 @@ def _post(self, cluster):
         self._check_cluster_quota_limit(context)
 
         temp_id = cluster.cluster_template_id
-        cluster_template = objects.ClusterTemplate.get_by_uuid(context,
-                                                               temp_id)
+        cluster_template = objects.ClusterTemplate.get(context, temp_id)
+        # We are not sure if we got a uuid or name here. So just set
+        # explicitly the uuid of the cluster template in the cluster.
+        cluster.cluster_template_id = cluster_template.uuid
         # If keypair not present, use cluster_template value
         if cluster.keypair is None:
             cluster.keypair = cluster_template.keypair_id

magnum/api/validation.py

@@ -34,11 +34,25 @@
 federation_update_allowed_properties = set(['member_ids', 'properties'])
 
 
+def ct_not_found_to_bad_request():
+    @decorator.decorator
+    def wrapper(func, *args, **kwargs):
+        try:
+            return func(*args, **kwargs)
+        except exception.ClusterTemplateNotFound as e:
+            # Change error code because 404 (NotFound) is inappropriate
+            # response for a POST request to create a Cluster
+            e.code = 400  # BadRequest
+            raise
+
+    return wrapper
+
+
 def enforce_cluster_type_supported():
     @decorator.decorator
     def wrapper(func, *args, **kwargs):
         cluster = args[1]
-        cluster_template = objects.ClusterTemplate.get_by_uuid(
+        cluster_template = objects.ClusterTemplate.get(
             pecan.request.context, cluster.cluster_template_id)
         cluster_type = (cluster_template.server_type,
                         cluster_template.cluster_distro,
@@ -77,7 +91,7 @@ def enforce_cluster_volume_storage_size():
     @decorator.decorator
     def wrapper(func, *args, **kwargs):
         cluster = args[1]
-        cluster_template = objects.ClusterTemplate.get_by_uuid(
+        cluster_template = objects.ClusterTemplate.get(
             pecan.request.context, cluster.cluster_template_id)
         _enforce_volume_storage_size(
             cluster_template.as_dict(), cluster.as_dict())

magnum/common/exception.py

@@ -480,3 +480,8 @@ class InvalidClusterTemplateForUpgrade(Conflict):
 
 class ClusterAPIAddressUnavailable(Conflict):
     message = _("Cluster API address is not available yet")
+
+
+class ObjectError(MagnumException):
+    message = _("Failed to perform action %{action}s on %{obj_name}s with "
+                "uuid %{obj_id}s: %{reason}s")

magnum/objects/cluster.py

@@ -22,6 +22,9 @@
 from magnum.objects.nodegroup import NodeGroup
 
 
+LAZY_LOADED_ATTRS = ['cluster_template']
+
+
 @base.MagnumObjectRegistry.register
 class Cluster(base.MagnumPersistentObject, base.MagnumObject,
               base.MagnumObjectDictCompat):
@@ -100,16 +103,11 @@ class Cluster(base.MagnumPersistentObject, base.MagnumObject,
     def _from_db_object(cluster, db_cluster):
         """Converts a database entity to a formal object."""
         for field in cluster.fields:
+            # cluster_template will be loaded lazily when it is needed
+            # by obj_load_attr.
             if field != 'cluster_template':
                 cluster[field] = db_cluster[field]
 
-        # Note(eliqiao): The following line needs to be placed outside the
-        # loop because there is a dependency from cluster_template to
-        # cluster_template_id. The cluster_template_id must be populated
-        # first in the loop before it can be used to find the cluster_template.
-        cluster['cluster_template'] = ClusterTemplate.get_by_uuid(
-            cluster._context, cluster.cluster_template_id)
-
         cluster.obj_reset_changes()
         return cluster
 
@@ -331,6 +329,17 @@ def refresh(self, context=None):
             if self.obj_attr_is_set(field) and self[field] != current[field]:
                 self[field] = current[field]
 
+    def obj_load_attr(self, attrname):
+        if attrname not in LAZY_LOADED_ATTRS:
+            raise exception.ObjectError(
+                action='obj_load_attr', obj_name=self.name, obj_id=self.uuid,
+                reason='unable to lazy-load %s' % attrname)
+
+        self['cluster_template'] = ClusterTemplate.get_by_uuid(
+            self._context, self.cluster_template_id)
+
+        self.obj_reset_changes(['cluster_template'])
+
     def as_dict(self):
         dict_ = super(Cluster, self).as_dict()
         # Update the dict with the attributes coming form

magnum/objects/cluster_template.py

@@ -14,7 +14,6 @@
 from oslo_utils import uuidutils
 from oslo_versionedobjects import fields
 
-from magnum.common import exception
 from magnum.db import api as dbapi
 from magnum.objects import base
 from magnum.objects import fields as m_fields
@@ -112,7 +111,7 @@ def get(cls, context, cluster_template_id):
         elif uuidutils.is_uuid_like(cluster_template_id):
             return cls.get_by_uuid(context, cluster_template_id)
         else:
-            raise exception.InvalidIdentity(identity=cluster_template_id)
+            return cls.get_by_name(context, cluster_template_id)
 
     @base.remotable_classmethod
     def get_by_id(cls, context, cluster_template_id):

magnum/tests/unit/api/controllers/v1/test_cluster.py

@@ -187,7 +187,9 @@ def test_get_all_with_pagination_marker(self):
 
     @mock.patch("magnum.common.policy.enforce")
     @mock.patch("magnum.common.context.make_context")
-    def test_get_all_with_all_projects(self, mock_context, mock_policy):
+    @mock.patch("magnum.objects.Cluster.obj_load_attr")
+    @mock.patch("magnum.objects.Cluster.cluster_template")
+    def test_get_all_with_all_projects(self, mock_context, mock_policy, mock_load, mock_template):
         for id_ in range(4):
             temp_uuid = uuidutils.generate_uuid()
             obj_utils.create_test_cluster(self.context, id=id_,

magnum/tests/unit/api/test_validation.py

@@ -30,7 +30,7 @@
 class TestValidation(base.BaseTestCase):
 
     def _test_enforce_cluster_type_supported(
-            self, mock_cluster_template_get_by_uuid, mock_cluster_get_by_uuid,
+            self, mock_cluster_template_get, mock_cluster_get_by_uuid,
             mock_pecan_request, cluster_type, assert_raised=False):
 
         @v.enforce_cluster_type_supported()
@@ -41,7 +41,7 @@ def test(self, cluster):
         cluster_template = obj_utils.get_test_cluster_template(
             mock_pecan_request.context, uuid='cluster_template_id',
             coe=coe, cluster_distro=cluster_distro, server_type=server_type)
-        mock_cluster_template_get_by_uuid.return_value = cluster_template
+        mock_cluster_template_get.return_value = cluster_template
 
         cluster = mock.MagicMock()
         cluster.cluster_template_id = 'cluster_template_id'
@@ -56,26 +56,26 @@ def test(self, cluster):
 
     @mock.patch('pecan.request')
     @mock.patch('magnum.objects.Cluster.get_by_uuid')
-    @mock.patch('magnum.objects.ClusterTemplate.get_by_uuid')
+    @mock.patch('magnum.objects.ClusterTemplate.get')
     def test_enforce_cluster_type_supported(
-            self, mock_cluster_template_get_by_uuid, mock_cluster_get_by_uuid,
+            self, mock_cluster_template_get, mock_cluster_get_by_uuid,
             mock_pecan_request):
 
         cluster_type = ('vm', 'fedora-atomic', 'kubernetes')
         self._test_enforce_cluster_type_supported(
-            mock_cluster_template_get_by_uuid, mock_cluster_get_by_uuid,
+            mock_cluster_template_get, mock_cluster_get_by_uuid,
             mock_pecan_request, cluster_type)
 
     @mock.patch('pecan.request')
     @mock.patch('magnum.objects.Cluster.get_by_uuid')
-    @mock.patch('magnum.objects.ClusterTemplate.get_by_uuid')
+    @mock.patch('magnum.objects.ClusterTemplate.get')
     def test_enforce_cluster_type_not_supported(
-            self, mock_cluster_template_get_by_uuid, mock_cluster_get_by_uuid,
+            self, mock_cluster_template_get, mock_cluster_get_by_uuid,
             mock_pecan_request):
 
         cluster_type = ('vm', 'foo', 'kubernetes')
         exc = self._test_enforce_cluster_type_supported(
-            mock_cluster_template_get_by_uuid, mock_cluster_get_by_uuid,
+            mock_cluster_template_get, mock_cluster_get_by_uuid,
             mock_pecan_request, cluster_type, assert_raised=True)
         self.assertEqual('Cluster type (vm, foo, kubernetes) not supported.',
                          exc.message)

magnum/tests/unit/objects/test_cluster.py

@@ -117,13 +117,12 @@ def test_list_all(self, mock_cluster_template_get):
             self.assertThat(clusters, HasLength(1))
             self.assertIsInstance(clusters[0], objects.Cluster)
             self.assertEqual(self.context, clusters[0]._context)
+            mock_cluster_template_get.assert_not_called()
 
-    @mock.patch('magnum.objects.ClusterTemplate.get_by_uuid')
-    def test_list_with_filters(self, mock_cluster_template_get):
+    def test_list_with_filters(self):
         with mock.patch.object(self.dbapi, 'get_cluster_list',
                                autospec=True) as mock_get_list:
             mock_get_list.return_value = [self.fake_cluster]
-            mock_cluster_template_get.return_value = self.fake_cluster_template
             filters = {'name': 'cluster1'}
             clusters = objects.Cluster.list(self.context, filters=filters)
 
@@ -136,24 +135,20 @@ def test_list_with_filters(self, mock_cluster_template_get):
             self.assertIsInstance(clusters[0], objects.Cluster)
             self.assertEqual(self.context, clusters[0]._context)
 
-    @mock.patch('magnum.objects.ClusterTemplate.get_by_uuid')
-    def test_create(self, mock_cluster_template_get):
+    def test_create(self):
         with mock.patch.object(self.dbapi, 'create_cluster',
                                autospec=True) as mock_create_cluster:
-            mock_cluster_template_get.return_value = self.fake_cluster_template
             mock_create_cluster.return_value = self.fake_cluster
             cluster = objects.Cluster(self.context, **self.fake_cluster)
             cluster.create()
             mock_create_cluster.assert_called_once_with(self.fake_cluster)
             self.assertEqual(self.context, cluster._context)
 
-    @mock.patch('magnum.objects.ClusterTemplate.get_by_uuid')
-    def test_destroy(self, mock_cluster_template_get):
+    def test_destroy(self):
         uuid = self.fake_cluster['uuid']
         with mock.patch.object(self.dbapi, 'get_cluster_by_uuid',
                                autospec=True) as mock_get_cluster:
             mock_get_cluster.return_value = self.fake_cluster
-            mock_cluster_template_get.return_value = self.fake_cluster_template
             with mock.patch.object(self.dbapi, 'destroy_cluster',
                                    autospec=True) as mock_destroy_cluster:
                 cluster = objects.Cluster.get_by_uuid(self.context, uuid)
@@ -162,12 +157,10 @@ def test_destroy(self, mock_cluster_template_get):
                 mock_destroy_cluster.assert_called_once_with(uuid)
                 self.assertEqual(self.context, cluster._context)
 
-    @mock.patch('magnum.objects.ClusterTemplate.get_by_uuid')
-    def test_save(self, mock_cluster_template_get):
+    def test_save(self):
         uuid = self.fake_cluster['uuid']
         with mock.patch.object(self.dbapi, 'get_cluster_by_uuid',
                                autospec=True) as mock_get_cluster:
-            mock_cluster_template_get.return_value = self.fake_cluster_template
             mock_get_cluster.return_value = self.fake_cluster
             with mock.patch.object(self.dbapi, 'update_cluster',
                                    autospec=True) as mock_update_cluster:
@@ -177,12 +170,10 @@ def test_save(self, mock_cluster_template_get):
 
                 mock_get_cluster.assert_called_once_with(self.context, uuid)
                 mock_update_cluster.assert_called_once_with(
-                    uuid, {'status': 'DELETE_IN_PROGRESS',
-                           'cluster_template': self.fake_cluster_template})
+                    uuid, {'status': 'DELETE_IN_PROGRESS'})
                 self.assertEqual(self.context, cluster._context)
 
-    @mock.patch('magnum.objects.ClusterTemplate.get_by_uuid')
-    def test_refresh(self, mock_cluster_template_get):
+    def test_refresh(self):
         uuid = self.fake_cluster['uuid']
         new_uuid = uuidutils.generate_uuid()
         returns = [dict(self.fake_cluster, uuid=uuid),
@@ -192,7 +183,6 @@ def test_refresh(self, mock_cluster_template_get):
         with mock.patch.object(self.dbapi, 'get_cluster_by_uuid',
                                side_effect=returns,
                                autospec=True) as mock_get_cluster:
-            mock_cluster_template_get.return_value = self.fake_cluster_template
             cluster = objects.Cluster.get_by_uuid(self.context, uuid)
             self.assertEqual(uuid, cluster.uuid)
             cluster.refresh()

magnum/tests/unit/objects/test_cluster_template.py

@@ -52,7 +52,7 @@ def test_get_by_uuid(self):
             self.assertEqual(self.context, cluster_template._context)
 
     def test_get_bad_id_and_uuid(self):
-        self.assertRaises(exception.InvalidIdentity,
+        self.assertRaises(exception.ClusterTemplateNotFound,
                           objects.ClusterTemplate.get, self.context,
                           'not-a-uuid')