From 3ca7eaab0287ce3f6d556baf0d1e0bb2f9d8aeb5 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Mon, 23 Oct 2017 12:17:32 -0400 Subject: [PATCH] Add Flavor.description attribute This adds the description attribute to the Flavor object and the accompanying database schema upgrade. This is the first field on a flavor that we've allowed to be updated, so it requires some new plumbing for doing an update on the flavor record itself during save(). The updateable fields are whitelisted so we don't leak other updates in here, like name, flavorid, vcpus, etc. As part of this change, we also have to be sure to not persist the description in the embedded instance.flavor since (1) it's a large field and (2) we are not going to expose the instance.flavor.description out of the servers API. Versioned notifications will be handled in a later change. Part of blueprint flavor-description Change-Id: I6db4eb46df0d7ec025b969a46621823957503958 --- .../versions/050_flavors_add_description.py | 26 +++++++++++ nova/db/sqlalchemy/api_models.py | 1 + nova/objects/flavor.py | 39 ++++++++++++++-- nova/objects/instance.py | 18 ++++++++ .../functional/db/api/test_migrations.py | 3 ++ nova/tests/functional/db/test_flavor.py | 1 + nova/tests/functional/db/test_flavor_model.py | 3 ++ nova/tests/functional/db/test_instance.py | 33 ++++++++++++++ .../openstack/compute/test_flavor_access.py | 3 +- nova/tests/unit/compute/test_compute_api.py | 1 + nova/tests/unit/db/fakes.py | 1 + .../unit/db/test_sqlalchemy_migration.py | 13 +++--- nova/tests/unit/fake_flavor.py | 3 +- nova/tests/unit/objects/test_flavor.py | 44 +++++++++++++++++++ nova/tests/unit/objects/test_objects.py | 2 +- 15 files changed, 179 insertions(+), 12 deletions(-) create mode 100644 nova/db/sqlalchemy/api_migrations/migrate_repo/versions/050_flavors_add_description.py diff --git a/nova/db/sqlalchemy/api_migrations/migrate_repo/versions/050_flavors_add_description.py b/nova/db/sqlalchemy/api_migrations/migrate_repo/versions/050_flavors_add_description.py new file mode 100644 index 0000000000..0529140200 --- /dev/null +++ b/nova/db/sqlalchemy/api_migrations/migrate_repo/versions/050_flavors_add_description.py @@ -0,0 +1,26 @@ +# 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. + +from sqlalchemy import Column +from sqlalchemy import MetaData +from sqlalchemy import Table +from sqlalchemy import Text + + +def upgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + flavors = Table('flavors', meta, autoload=True) + + if not hasattr(flavors.c, 'description'): + flavors.create_column(Column('description', Text())) diff --git a/nova/db/sqlalchemy/api_models.py b/nova/db/sqlalchemy/api_models.py index 2d42036928..3982606996 100644 --- a/nova/db/sqlalchemy/api_models.py +++ b/nova/db/sqlalchemy/api_models.py @@ -190,6 +190,7 @@ class Flavors(API_BASE): vcpu_weight = Column(Integer) disabled = Column(Boolean, default=False) is_public = Column(Boolean, default=True) + description = Column(Text) class FlavorExtraSpecs(API_BASE): diff --git a/nova/objects/flavor.py b/nova/objects/flavor.py index f9bb32e252..8c4d8e2f7e 100644 --- a/nova/objects/flavor.py +++ b/nova/objects/flavor.py @@ -15,6 +15,7 @@ from oslo_db import exception as db_exc from oslo_db.sqlalchemy import utils as sqlalchemyutils from oslo_log import log as logging +from oslo_utils import versionutils from sqlalchemy import or_ from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import asc @@ -37,6 +38,9 @@ OPTIONAL_FIELDS = ['extra_specs', 'projects'] # Remove these fields in version 2.0 of the object. DEPRECATED_FIELDS = ['deleted', 'deleted_at'] +# Non-joined fields which can be updated. +MUTABLE_FIELDS = set(['description']) + CONF = nova.conf.CONF @@ -197,7 +201,9 @@ class Flavor(base.NovaPersistentObject, base.NovaObject, # Version 1.0: Initial version # Version 1.1: Added save_projects(), save_extra_specs(), removed # remotable from save() - VERSION = '1.1' + # Version 1.2: Added description field. Note: this field should not be + # persisted with the embedded instance.flavor. + VERSION = '1.2' fields = { 'id': fields.IntegerField(), @@ -214,6 +220,7 @@ class Flavor(base.NovaPersistentObject, base.NovaObject, 'is_public': fields.BooleanField(), 'extra_specs': fields.DictOfStringsField(), 'projects': fields.ListOfStringsField(), + 'description': fields.StringField(nullable=True) } def __init__(self, *args, **kwargs): @@ -221,6 +228,12 @@ class Flavor(base.NovaPersistentObject, base.NovaObject, self._orig_extra_specs = {} self._orig_projects = [] + def obj_make_compatible(self, primitive, target_version): + super(Flavor, self).obj_make_compatible(primitive, target_version) + target_version = versionutils.convert_version_to_tuple(target_version) + if target_version < (1, 2) and 'description' in primitive: + del primitive['description'] + @staticmethod def _from_db_object(context, flavor, db_flavor, expected_attrs=None): if expected_attrs is None: @@ -473,13 +486,30 @@ class Flavor(base.NovaPersistentObject, base.NovaObject, self._flavor_extra_specs_del(self._context, self.id, key) self.obj_reset_changes(['extra_specs']) + # NOTE(mriedem): This method is not remotable since we only expect the API + # to be able to make updates to a flavor. + @db_api.api_context_manager.writer + def _save(self, context, values): + db_flavor = context.session.query(api_models.Flavors).\ + filter_by(id=self.id).first() + if not db_flavor: + raise exception.FlavorNotFound(flavor_id=self.id) + db_flavor.update(values) + db_flavor.save(context.session) + # Refresh ourselves from the DB object so we get the new updated_at. + self._from_db_object(context, self, db_flavor) + self.obj_reset_changes() + def save(self): updates = self.obj_get_changes() projects = updates.pop('projects', None) extra_specs = updates.pop('extra_specs', None) if updates: - raise exception.ObjectActionError( - action='save', reason='read-only fields were changed') + # Only allowed to update from the whitelist of mutable fields. + if set(updates.keys()) - MUTABLE_FIELDS: + raise exception.ObjectActionError( + action='save', reason='read-only fields were changed') + self._save(self._context, updates) if extra_specs is not None: deleted_keys = (set(self._orig_extra_specs.keys()) - @@ -505,7 +535,8 @@ class Flavor(base.NovaPersistentObject, base.NovaObject, if added_projects or deleted_projects: self.save_projects(added_projects, deleted_projects) - if added_keys or deleted_keys or added_projects or deleted_projects: + if (added_keys or deleted_keys or added_projects or deleted_projects or + updates): self._send_notification(fields.NotificationAction.UPDATE) @staticmethod diff --git a/nova/objects/instance.py b/nova/objects/instance.py index 3c166addd2..aca128c14a 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -539,6 +539,7 @@ class Instance(base.NovaPersistentObject, base.NovaObject, 'old': old, 'new': new, } + self._nullify_flavor_description(flavor_info) updates['extra']['flavor'] = jsonutils.dumps(flavor_info) keypairs = updates.pop('keypairs', None) if keypairs is not None: @@ -625,6 +626,22 @@ class Instance(base.NovaPersistentObject, base.NovaObject, # NOTE(gibi): tags are not saved through the instance pass + @staticmethod + def _nullify_flavor_description(flavor_info): + """Helper method to nullify descriptions from a set of primitive + flavors. + + Note that we don't remove the flavor description since that would + make the versioned notification FlavorPayload have to handle the field + not being set on the embedded instance.flavor. + + :param dict: dict of primitive flavor objects where the values are the + flavors which get persisted in the instance_extra.flavor table. + """ + for flavor in flavor_info.values(): + if flavor and 'description' in flavor['nova_object.data']: + flavor['nova_object.data']['description'] = None + def _save_flavor(self, context): if not any([x in self.obj_what_changed() for x in ('flavor', 'old_flavor', 'new_flavor')]): @@ -636,6 +653,7 @@ class Instance(base.NovaPersistentObject, base.NovaObject, 'new': (self.new_flavor and self.new_flavor.obj_to_primitive() or None), } + self._nullify_flavor_description(flavor_info) self._extra_values_to_save['flavor'] = jsonutils.dumps(flavor_info) self.obj_reset_changes(['flavor', 'old_flavor', 'new_flavor']) diff --git a/nova/tests/functional/db/api/test_migrations.py b/nova/tests/functional/db/api/test_migrations.py index 171edbe530..4bb1f52b23 100644 --- a/nova/tests/functional/db/api/test_migrations.py +++ b/nova/tests/functional/db/api/test_migrations.py @@ -643,6 +643,9 @@ class NovaAPIMigrationsWalk(test_migrations.WalkVersionsMixin): 'consumers_project_id_user_id_uuid_idx', ) + def _check_050(self, engine, data): + self.assertColumnExists(engine, 'flavors', 'description') + class TestNovaAPIMigrationsWalkSQLite(NovaAPIMigrationsWalk, test_base.DbTestCase, diff --git a/nova/tests/functional/db/test_flavor.py b/nova/tests/functional/db/test_flavor.py index 7709bb9142..71c1a2233e 100644 --- a/nova/tests/functional/db/test_flavor.py +++ b/nova/tests/functional/db/test_flavor.py @@ -35,6 +35,7 @@ fake_api_flavor = { 'is_public': True, 'extra_specs': {'foo': 'bar'}, 'projects': ['project1', 'project2'], + 'description': None } diff --git a/nova/tests/functional/db/test_flavor_model.py b/nova/tests/functional/db/test_flavor_model.py index 5cf484c054..447c2268e6 100644 --- a/nova/tests/functional/db/test_flavor_model.py +++ b/nova/tests/functional/db/test_flavor_model.py @@ -32,6 +32,9 @@ class FlavorTablesCompareTestCase(test.NoDBTestCase): flavors = api_models.Flavors() instance_types = models.InstanceTypes() columns_flavors = self._get_columns_list(flavors) + # The description column is only in the API database so we have to + # exclude it from this check. + columns_flavors.remove('description') columns_instance_types = self._get_columns_list(instance_types) self.assertTrue(self._check_column_list(columns_flavors, columns_instance_types)) diff --git a/nova/tests/functional/db/test_instance.py b/nova/tests/functional/db/test_instance.py index 75fcce8577..9e800c755d 100644 --- a/nova/tests/functional/db/test_instance.py +++ b/nova/tests/functional/db/test_instance.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_utils import uuidutils + from nova.compute import vm_states from nova import context from nova import objects @@ -40,3 +42,34 @@ class InstanceObjectTestCase(test.TestCase): self.context, self.context.project_id, self.context.user_id, vm_states.ACTIVE) self.assertEqual(1, count) + + def test_embedded_instance_flavor_description_is_not_persisted(self): + """The instance.flavor.description field will not be exposed out + of the REST API when showing server details, so we want to make + sure the embedded instance.flavor.description is not persisted with + the instance_extra.flavor information. + """ + # Create a flavor with a description. + flavorid = uuidutils.generate_uuid() + flavor = objects.Flavor(context.get_admin_context(), + name=flavorid, flavorid=flavorid, + memory_mb=2048, vcpus=2, + description='do not persist me in an instance') + flavor.create() + + # Now create the instance with that flavor. + instance = self._create_instance(flavor=flavor) + + # Make sure the embedded flavor.description is nulled out. + self.assertIsNone(instance.flavor.description) + + # Now set the flavor on the instance again to make sure save() does + # not persist the flavor.description value. + instance.flavor = flavor + self.assertIn('flavor', list(instance.obj_what_changed())) + instance.save() + + # Get the instance from the database since our old version is dirty. + instance = objects.Instance.get_by_uuid( + self.context, instance.uuid, expected_attrs=['flavor']) + self.assertIsNone(instance.flavor.description) diff --git a/nova/tests/unit/api/openstack/compute/test_flavor_access.py b/nova/tests/unit/api/openstack/compute/test_flavor_access.py index ef2f8048c3..20a73ef996 100644 --- a/nova/tests/unit/api/openstack/compute/test_flavor_access.py +++ b/nova/tests/unit/api/openstack/compute/test_flavor_access.py @@ -44,7 +44,8 @@ def generate_flavor(flavorid, ispublic): 'disabled': False, 'extra_specs': {}, 'vcpu_weight': None, - 'is_public': bool(ispublic) + 'is_public': bool(ispublic), + 'description': None } diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 8a773e7680..e8a66186e3 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -105,6 +105,7 @@ class _ComputeAPIUnitTestMixIn(object): 'created_at': datetime.datetime(2012, 1, 19, 18, 49, 30, 877329), 'updated_at': None, + 'description': None } if updates: flavor.update(updates) diff --git a/nova/tests/unit/db/fakes.py b/nova/tests/unit/db/fakes.py index c0ce7e36cc..b033fecada 100644 --- a/nova/tests/unit/db/fakes.py +++ b/nova/tests/unit/db/fakes.py @@ -362,6 +362,7 @@ def stub_out_db_instance_api(test, injected=True): 'disabled': False, 'is_public': True, 'extra_specs': {}, + 'description': None } if updates: instance_type.update(updates) diff --git a/nova/tests/unit/db/test_sqlalchemy_migration.py b/nova/tests/unit/db/test_sqlalchemy_migration.py index 8f4f7cdedf..c12d838583 100644 --- a/nova/tests/unit/db/test_sqlalchemy_migration.py +++ b/nova/tests/unit/db/test_sqlalchemy_migration.py @@ -382,11 +382,14 @@ class TestNewtonCellsCheck(test.NoDBTestCase): self.engine = db_api.get_api_engine() def _flavor_me(self): - flavor = objects.Flavor(context=self.context, - name='foo', memory_mb=123, - vcpus=1, root_gb=1, - flavorid='m1.foo') - flavor.create() + # We can't use the Flavor object or model to create the flavor because + # the model and object have the description field now but at this point + # we have not run the migration schema to add the description column. + flavors = db_utils.get_table(self.engine, 'flavors') + values = dict(name='foo', memory_mb=123, + vcpus=1, root_gb=1, + flavorid='m1.foo', swap=0) + flavors.insert().execute(values) def test_upgrade_with_no_cell_mappings(self): self._flavor_me() diff --git a/nova/tests/unit/fake_flavor.py b/nova/tests/unit/fake_flavor.py index dd1b0b15a8..178835944b 100644 --- a/nova/tests/unit/fake_flavor.py +++ b/nova/tests/unit/fake_flavor.py @@ -29,7 +29,8 @@ def fake_db_flavor(**updates): 'disabled': False, 'is_public': True, 'extra_specs': {}, - 'projects': [] + 'projects': [], + 'description': None } for name, field in objects.Flavor.fields.items(): diff --git a/nova/tests/unit/objects/test_flavor.py b/nova/tests/unit/objects/test_flavor.py index e6291cf29c..1162f3be0f 100644 --- a/nova/tests/unit/objects/test_flavor.py +++ b/nova/tests/unit/objects/test_flavor.py @@ -16,7 +16,9 @@ import datetime import mock from oslo_db import exception as db_exc +from oslo_utils import uuidutils +from nova import context as nova_context from nova.db.sqlalchemy import api as db_api from nova.db.sqlalchemy import api_models from nova import exception @@ -41,6 +43,7 @@ fake_flavor = { 'disabled': False, 'is_public': True, 'extra_specs': {'foo': 'bar'}, + 'description': None } @@ -321,6 +324,47 @@ class TestFlavor(test_objects._LocalTest, _TestFlavor): flavor = objects.Flavor.get_by_id(self.context, db_flavor['id']) self.assertEqual({'marty': 'mcfly'}, flavor.extra_specs) + # NOTE(mriedem): There is no remotable method for updating the description + # in a flavor so we test this local-only. + @mock.patch('nova.objects.Flavor._send_notification') + def test_description(self, mock_notify): + # Create a flavor with a description. + ctxt = nova_context.get_admin_context() + flavorid = uuidutils.generate_uuid() + dict_flavor = dict(fake_flavor, name=flavorid, flavorid=flavorid) + del dict_flavor['id'] + flavor = flavor_obj.Flavor(ctxt, **dict_flavor) + flavor.description = 'rainbows and unicorns' + flavor.create() + mock_notify.assert_called_once_with('create') + # Lookup the flavor to make sure the description is set. + flavor = flavor_obj.Flavor.get_by_flavor_id(ctxt, flavorid) + self.assertEqual('rainbows and unicorns', flavor.description) + + # Now reset the flavor.description since it's nullable=True. + mock_notify.reset_mock() + self.assertEqual(0, len(flavor.obj_what_changed()), + flavor.obj_what_changed()) + flavor.description = None + self.assertEqual(['description'], list(flavor.obj_what_changed()), + flavor.obj_what_changed()) + old_updated_at = flavor.updated_at + flavor.save() + # Make sure we reloaded the flavor from the database. + self.assertNotEqual(old_updated_at, flavor.updated_at) + mock_notify.assert_called_once_with('update') + self.assertEqual(0, len(flavor.obj_what_changed()), + flavor.obj_what_changed()) + # Lookup the flavor to make sure the description is gone. + flavor = flavor_obj.Flavor.get_by_flavor_id(ctxt, flavorid) + self.assertIsNone(flavor.description) + + # Test compatibility. + flavor.description = 'flavor descriptions are not backward compatible' + flavor_primitive = flavor.obj_to_primitive() + flavor.obj_make_compatible(flavor_primitive, '1.1') + self.assertNotIn('description', flavor_primitive) + class TestFlavorRemote(test_objects._RemoteTest, _TestFlavor): pass diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 360c0c6326..a9d8a22e53 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1087,7 +1087,7 @@ object_data = { 'EC2VolumeMapping': '1.0-5b713751d6f97bad620f3378a521020d', 'FixedIP': '1.14-53e1c10b539f1a82fe83b1af4720efae', 'FixedIPList': '1.15-07b6261cef836cb09d2d8673f68ece15', - 'Flavor': '1.1-b6bb7a730a79d720344accefafacf7ee', + 'Flavor': '1.2-4ce99b41327bb230262e5a8f45ff0ce3', 'FlavorList': '1.1-52b5928600e7ca973aa4fc1e46f3934c', 'FloatingIP': '1.10-52a67d52d85eb8b3f324a5b7935a335b', 'FloatingIPList': '1.12-e4debd21fddb12cf40d36f737225fa9d',