From 0792885a1b2c5f5d93f48bd07b9a169c2096669f Mon Sep 17 00:00:00 2001 From: Stavros Moiras Date: Wed, 21 Apr 2021 14:43:37 +0200 Subject: [PATCH] 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 Change-Id: I2313c6a8b647e521cfa476f9cec65ab286fa5a23 --- magnum/api/controllers/v1/cluster.py | 31 ++++--------------- magnum/api/validation.py | 18 +++++++++-- magnum/common/exception.py | 5 +++ magnum/objects/cluster.py | 23 +++++++++----- magnum/objects/cluster_template.py | 3 +- .../unit/api/controllers/v1/test_cluster.py | 4 ++- magnum/tests/unit/api/test_validation.py | 16 +++++----- magnum/tests/unit/objects/test_cluster.py | 24 +++++--------- .../unit/objects/test_cluster_template.py | 2 +- 9 files changed, 63 insertions(+), 63 deletions(-) mode change 100644 => 100755 magnum/api/validation.py mode change 100644 => 100755 magnum/objects/cluster.py mode change 100644 => 100755 magnum/objects/cluster_template.py mode change 100644 => 100755 magnum/tests/unit/api/controllers/v1/test_cluster.py mode change 100644 => 100755 magnum/tests/unit/api/test_validation.py mode change 100644 => 100755 magnum/tests/unit/objects/test_cluster.py mode change 100644 => 100755 magnum/tests/unit/objects/test_cluster_template.py diff --git a/magnum/api/controllers/v1/cluster.py b/magnum/api/controllers/v1/cluster.py index dccf10f874..fde06a68e0 100755 --- a/magnum/api/controllers/v1/cluster.py +++ b/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 @@ class Cluster(base.APIBase): """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 @@ class ClustersController(base.Controller): @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 @@ class ClustersController(base.Controller): 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 diff --git a/magnum/api/validation.py b/magnum/api/validation.py old mode 100644 new mode 100755 index e04a2236d3..de9b096276 --- a/magnum/api/validation.py +++ b/magnum/api/validation.py @@ -34,11 +34,25 @@ cluster_update_allowed_properties = set(['node_count', 'health_status', 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()) diff --git a/magnum/common/exception.py b/magnum/common/exception.py index 9a7e6f3b44..81781191d8 100755 --- a/magnum/common/exception.py +++ b/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") diff --git a/magnum/objects/cluster.py b/magnum/objects/cluster.py old mode 100644 new mode 100755 index 896006b1d2..a65321b97b --- a/magnum/objects/cluster.py +++ b/magnum/objects/cluster.py @@ -22,6 +22,9 @@ from magnum.objects import fields as m_fields 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 @@ class Cluster(base.MagnumPersistentObject, base.MagnumObject, 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 diff --git a/magnum/objects/cluster_template.py b/magnum/objects/cluster_template.py old mode 100644 new mode 100755 index de25577b68..d352feb6dd --- a/magnum/objects/cluster_template.py +++ b/magnum/objects/cluster_template.py @@ -14,7 +14,6 @@ from oslo_utils import strutils 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 @@ class ClusterTemplate(base.MagnumPersistentObject, base.MagnumObject, 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): diff --git a/magnum/tests/unit/api/controllers/v1/test_cluster.py b/magnum/tests/unit/api/controllers/v1/test_cluster.py old mode 100644 new mode 100755 index ea0c6e1e5f..4e5132d19b --- a/magnum/tests/unit/api/controllers/v1/test_cluster.py +++ b/magnum/tests/unit/api/controllers/v1/test_cluster.py @@ -187,7 +187,9 @@ class TestListCluster(api_base.FunctionalTest): @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_, diff --git a/magnum/tests/unit/api/test_validation.py b/magnum/tests/unit/api/test_validation.py old mode 100644 new mode 100755 index e6d89e3129..91d9f8f9b0 --- a/magnum/tests/unit/api/test_validation.py +++ b/magnum/tests/unit/api/test_validation.py @@ -30,7 +30,7 @@ CONF = magnum.conf.CONF 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 @@ class TestValidation(base.BaseTestCase): 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 @@ class TestValidation(base.BaseTestCase): @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) diff --git a/magnum/tests/unit/objects/test_cluster.py b/magnum/tests/unit/objects/test_cluster.py old mode 100644 new mode 100755 index 10abab59d2..186de31824 --- a/magnum/tests/unit/objects/test_cluster.py +++ b/magnum/tests/unit/objects/test_cluster.py @@ -117,13 +117,12 @@ class TestClusterObject(base.DbTestCase): 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 @@ class TestClusterObject(base.DbTestCase): 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 @@ class TestClusterObject(base.DbTestCase): 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 @@ class TestClusterObject(base.DbTestCase): 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 @@ class TestClusterObject(base.DbTestCase): 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() diff --git a/magnum/tests/unit/objects/test_cluster_template.py b/magnum/tests/unit/objects/test_cluster_template.py old mode 100644 new mode 100755 index dc868b5772..4df97b9296 --- a/magnum/tests/unit/objects/test_cluster_template.py +++ b/magnum/tests/unit/objects/test_cluster_template.py @@ -52,7 +52,7 @@ class TestClusterTemplateObject(base.DbTestCase): 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')