Merge "Optimize cluster list api"

Author: Zuul

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