openstack
/
magnum
Code Issues Proposed changes

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 <stavros.moiras@cern.ch>

Change-Id: I2313c6a8b647e521cfa476f9cec65ab286fa5a23
This commit is contained in:
Stavros Moiras 2021-04-21 14:43:37 +02:00
parent bc6ec3ab63
commit 0792885a1b
9 changed files with 63 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
magnum/api/controllers/v1/cluster.py
View File

@ -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

18
magnum/api/validation.py Normal file → Executable file
View File

@ -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())

5
magnum/common/exception.py
View File

@ -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")

23
magnum/objects/cluster.py Normal file → Executable file
View File

@ -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

3
magnum/objects/cluster_template.py Normal file → Executable file
View File

@ -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):

4
magnum/tests/unit/api/controllers/v1/test_cluster.py Normal file → Executable file
View File

@ -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_,

16
magnum/tests/unit/api/test_validation.py Normal file → Executable file
View File

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

24
magnum/tests/unit/objects/test_cluster.py Normal file → Executable file
View File

@ -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()

2
magnum/tests/unit/objects/test_cluster_template.py Normal file → Executable file
View File

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