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

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

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

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

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

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,

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
     }
 
 

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

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)

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
     }
 
 

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)

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)

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

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

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

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