Adds disabled field for instance-types.

The `disabled` field is intended to be used when phasing out
instance-types. In this case, a delete wouldn't work because the
instance-type needs to still be available for live instances using that
type, but we don't want to allow *new* instances created from that type.

In addition, we don't want to list hidden instance-types for regular
users, but we *do* want to list them for admin-users to ensure they have
a complete view of what's going on in the system.

Once all references to the phased-out instance-type have been dropped,
it would be safe to mark the instance-type as `deleted=True`.

Change-Id: I2af1c027f4d8114aee31353007dfdd3d0bb679ed

nova/api/openstack/compute/contrib/flavorextradata.py

@@ -37,7 +37,8 @@
 class FlavorextradataController(wsgi.Controller):
     def _get_flavor_refs(self):
         """Return a dictionary mapping flavorid to flavor_ref."""
-        flavor_refs = instance_types.get_all_types(True)
+
+        flavor_refs = instance_types.get_all_types(inactive=True)
         rval = {}
         for name, obj in flavor_refs.iteritems():
             rval[obj['flavorid']] = obj

nova/api/openstack/compute/flavors.py

@@ -94,6 +94,11 @@ def show(self, req, id):
     def _get_flavors(self, req):
         """Helper function that returns a list of flavor dicts."""
         filters = {}
+
+        context = req.environ['nova.context']
+        if not context.is_admin:
+            filters['disabled'] = False
+
         if 'minRam' in req.params:
             try:
                 filters['min_memory_mb'] = int(req.params['minRam'])

nova/api/openstack/compute/servers.py

@@ -704,6 +704,8 @@ def create(self, req, body):
                                                 headers={'Retry-After': 0})
         except exception.InstanceTypeMemoryTooSmall as error:
             raise exc.HTTPBadRequest(explanation=unicode(error))
+        except exception.InstanceTypeNotFound as error:
+            raise exc.HTTPBadRequest(explanation=unicode(error))
         except exception.InstanceTypeDiskTooSmall as error:
             raise exc.HTTPBadRequest(explanation=unicode(error))
         except exception.InvalidMetadata as error:

nova/api/openstack/compute/views/flavors.py

@@ -34,7 +34,7 @@ def basic(self, request, flavor):
         }
 
     def show(self, request, flavor):
-        return {
+        flavor_dict = {
             "flavor": {
                 "id": flavor["flavorid"],
                 "name": flavor["name"],
@@ -49,6 +49,14 @@ def show(self, request, flavor):
             },
         }
 
+        # NOTE(sirp): disabled attribute is namespaced for now for
+        # compatability with the OpenStack API. This should ultimately be made
+        # a first class attribute.
+        flavor_dict["flavor"]["OS-FLV-DISABLED:disabled"] =\
+                flavor.get("disabled", "")
+
+        return flavor_dict
+
     def index(self, request, flavors):
         """Return the 'index' view of flavors."""
         return self._list_view(self.basic, request, flavors)

nova/compute/api.py

@@ -356,6 +356,10 @@ def _create_instance(self, context, instance_type,
 
         block_device_mapping = block_device_mapping or []
 
+        if instance_type['disabled']:
+            raise exception.InstanceTypeNotFound(
+                    instance_type_id=instance_type['id'])
+
         # Check quotas
         num_instances, quota_reservations = self._check_num_instances_quota(
                 context, instance_type, min_count, max_count)
@@ -1511,9 +1515,19 @@ def resize(self, context, instance, flavor_id=None, **kwargs):
         new_instance_type_name = new_instance_type['name']
         LOG.debug(_("Old instance type %(current_instance_type_name)s, "
                 " new instance type %(new_instance_type_name)s") % locals())
+
+        # FIXME(sirp): both of these should raise InstanceTypeNotFound instead
         if not new_instance_type:
             raise exception.FlavorNotFound(flavor_id=flavor_id)
 
+        same_instance_type = (current_instance_type['id'] ==
+                              new_instance_type['id'])
+
+        # NOTE(sirp): We don't want to force a customer to change their flavor
+        # when Ops is migrating off of a failed host.
+        if new_instance_type['disabled'] and not same_instance_type:
+            raise exception.FlavorNotFound(flavor_id=flavor_id)
+
         # NOTE(markwash): look up the image early to avoid auth problems later
         image = self.image_service.show(context, instance['image_ref'])
 

nova/compute/instance_types.py

@@ -97,14 +97,15 @@ def destroy(name):
         raise exception.InstanceTypeNotFoundByName(instance_type_name=name)
 
 
-def get_all_types(inactive=0, filters=None):
+def get_all_types(inactive=False, filters=None):
     """Get all non-deleted instance_types.
 
     Pass true as argument if you want deleted instance types returned also.
-
     """
     ctxt = context.get_admin_context()
-    inst_types = db.instance_type_get_all(ctxt, inactive, filters)
+    inst_types = db.instance_type_get_all(
+            ctxt, inactive=inactive, filters=filters)
+
     inst_type_dict = {}
     for inst_type in inst_types:
         inst_type_dict[inst_type['name']] = inst_type

nova/db/sqlalchemy/api.py

@@ -4007,16 +4007,27 @@ def instance_type_get_all(context, inactive=False, filters=None):
     Returns all instance types.
     """
     filters = filters or {}
+
+    # FIXME(sirp): now that we have the `disabled` field for instance-types, we
+    # should probably remove the use of `deleted` to mark inactive. `deleted`
+    # should mean truly deleted, e.g. we can safely purge the record out of the
+    # database.
     read_deleted = "yes" if inactive else "no"
+
     query = _instance_type_get_query(context, read_deleted=read_deleted)
 
     if 'min_memory_mb' in filters:
         query = query.filter(
                 models.InstanceTypes.memory_mb >= filters['min_memory_mb'])
+
     if 'min_root_gb' in filters:
         query = query.filter(
                 models.InstanceTypes.root_gb >= filters['min_root_gb'])
 
+    if 'disabled' in filters:
+        query = query.filter(
+                models.InstanceTypes.disabled == filters['disabled'])
+
     inst_types = query.order_by("name").all()
 
     return [_dict_with_extra_specs(i) for i in inst_types]

nova/db/sqlalchemy/migrate_repo/versions/099_add_disabled_instance_types.py

@@ -0,0 +1,40 @@
+# Copyright 2012 OpenStack LLC.
+#
+#    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 Boolean, Column, MetaData, Table
+
+from nova import log as logging
+
+LOG = logging.getLogger(__name__)
+
+
+def upgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+
+    instance_types = Table('instance_types', meta, autoload=True)
+    disabled = Column('disabled', Boolean)
+
+    instance_types.create_column(disabled)
+    instance_types.update().values(disabled=False).execute()
+
+
+def downgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+
+    instance_types = Table('instance_types', meta, autoload=True)
+    disabled = Column('disabled', Boolean)
+
+    instance_types.drop_column(disabled)

nova/db/sqlalchemy/models.py

@@ -313,6 +313,7 @@ class InstanceTypes(BASE, NovaBase):
     swap = Column(Integer, nullable=False, default=0)
     rxtx_factor = Column(Float, nullable=False, default=1)
     vcpu_weight = Column(Integer, nullable=True)
+    disabled = Column(Boolean, default=False)
 
     instances = relationship(Instance,
                            backref=backref('instance_type', uselist=False),

nova/test.py

@@ -291,3 +291,30 @@ def assertNotIn(self, a, b, *args, **kwargs):
             self.assertFalse(a in b, *args, **kwargs)
         else:
             f(a, b, *args, **kwargs)
+
+    def assertNotRaises(self, exc_class, func, *args, **kwargs):
+        """Assert that a particular exception is not raised.
+
+        If exc_class is None, then we assert that *no* error is raised.
+
+        Otherwise, we assert that only a particular error wasn't raised;
+        if any different exceptions were raised, we just silently capture
+        them and return.
+        """
+        exc_msg = kwargs.pop('exc_msg', '')
+
+        if exc_class is None:
+            # Ensure no errors were raised
+            try:
+                return func(*args, **kwargs)
+            except Exception:
+                raise
+                raise AssertionError(exc_msg)
+        else:
+            # Ensure a specific error wasn't raised
+            try:
+                return func(*args, **kwargs)
+            except exc_class:
+                raise AssertionError(exc_msg)
+            except Exception:
+                pass  # Any other errors are fine

nova/tests/api/openstack/compute/test_flavors.py

@@ -1,6 +1,6 @@
 # vim: tabstop=4 shiftwidth=4 softtabstop=4
 
-# Copyright 2010 OpenStack LLC.
+# Copyright 2012 OpenStack LLC.
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -23,6 +23,8 @@
 from nova.api.openstack.compute import flavors
 from nova.api.openstack import xmlutil
 import nova.compute.instance_types
+from nova import context
+from nova import db
 from nova import exception
 from nova import flags
 from nova import test
@@ -42,13 +44,15 @@
         "flavorid": '1',
         "name": 'flavor 1',
         "memory_mb": '256',
-        "root_gb": '10'
+        "root_gb": '10',
+        "disabled": False,
     },
     'flavor 2': {
         "flavorid": '2',
         "name": 'flavor 2',
         "memory_mb": '512',
-        "root_gb": '20'
+        "root_gb": '20',
+        "disabled": False,
     },
 }
 
@@ -110,6 +114,7 @@ def test_get_flavor_by_id(self):
         expected = {
             "flavor": {
                 "id": "1",
+                "OS-FLV-DISABLED:disabled": False,
                 "name": "flavor 1",
                 "ram": "256",
                 "disk": "10",
@@ -138,6 +143,7 @@ def test_get_flavor_with_custom_link_prefix(self):
         expected = {
             "flavor": {
                 "id": "1",
+                "OS-FLV-DISABLED:disabled": False,
                 "name": "flavor 1",
                 "ram": "256",
                 "disk": "10",
@@ -308,6 +314,7 @@ def test_get_flavor_list_detail(self):
             "flavors": [
                 {
                     "id": "1",
+                    "OS-FLV-DISABLED:disabled": False,
                     "name": "flavor 1",
                     "ram": "256",
                     "disk": "10",
@@ -327,6 +334,7 @@ def test_get_flavor_list_detail(self):
                 },
                 {
                     "id": "2",
+                    "OS-FLV-DISABLED:disabled": False,
                     "name": "flavor 2",
                     "ram": "512",
                     "disk": "20",
@@ -428,6 +436,7 @@ def test_get_flavor_list_detail_min_ram_and_min_disk(self):
             "flavors": [
                 {
                     "id": "2",
+                    "OS-FLV-DISABLED:disabled": False,
                     "name": "flavor 2",
                     "ram": "512",
                     "disk": "20",
@@ -703,3 +712,98 @@ def test_index_empty(self):
         xmlutil.validate_schema(root, 'flavors_index')
         flavor_elems = root.findall('{0}flavor'.format(NS))
         self.assertEqual(len(flavor_elems), 0)
+
+
+class DisabledFlavorsWithRealDBTest(test.TestCase):
+    """
+    Tests that disabled flavors should not be shown nor listed.
+    """
+    def setUp(self):
+        super(DisabledFlavorsWithRealDBTest, self).setUp()
+        self.controller = flavors.Controller()
+
+        # Add a new disabled type to the list of instance_types/flavors
+        self.req = fakes.HTTPRequest.blank('/v2/fake/flavors')
+        self.context = self.req.environ['nova.context']
+        self.admin_context = context.get_admin_context()
+
+        self.disabled_type = self._create_disabled_instance_type()
+        self.inst_types = db.api.instance_type_get_all(
+                self.admin_context)
+
+    def tearDown(self):
+        db.api.instance_type_destroy(
+                self.admin_context, self.disabled_type['name'])
+
+        super(DisabledFlavorsWithRealDBTest, self).tearDown()
+
+    def _create_disabled_instance_type(self):
+        inst_types = db.api.instance_type_get_all(
+                self.admin_context)
+
+        inst_type = inst_types[0]
+
+        del inst_type['id']
+        inst_type['name'] += '.disabled'
+        inst_type['flavorid'] = unicode(max(
+                [int(flavor['flavorid']) for flavor in inst_types]) + 1)
+        inst_type['disabled'] = True
+
+        disabled_type = db.api.instance_type_create(
+                self.admin_context, inst_type)
+
+        return disabled_type
+
+    def test_index_should_not_list_disabled_flavors_to_user(self):
+        self.context.is_admin = False
+
+        flavor_list = self.controller.index(self.req)['flavors']
+        api_flavorids = set(f['id'] for f in flavor_list)
+
+        db_flavorids = set(i['flavorid'] for i in self.inst_types)
+        disabled_flavorid = str(self.disabled_type['flavorid'])
+
+        self.assert_(disabled_flavorid in db_flavorids)
+        self.assertEqual(db_flavorids - set([disabled_flavorid]),
+                         api_flavorids)
+
+    def test_index_should_list_disabled_flavors_to_admin(self):
+        self.context.is_admin = True
+
+        flavor_list = self.controller.index(self.req)['flavors']
+        api_flavorids = set(f['id'] for f in flavor_list)
+
+        db_flavorids = set(i['flavorid'] for i in self.inst_types)
+        disabled_flavorid = str(self.disabled_type['flavorid'])
+
+        self.assert_(disabled_flavorid in db_flavorids)
+        self.assertEqual(db_flavorids, api_flavorids)
+
+    def test_show_should_include_disabled_flavor_for_user(self):
+        """
+        Counterintuitively we should show disabled flavors to all users and not
+        just admins. The reason is that, when a user performs a server-show
+        request, we want to be able to display the pretty flavor name ('512 MB
+        Instance') and not just the flavor-id even if the flavor id has been
+        marked disabled.
+        """
+        self.context.is_admin = False
+
+        flavor = self.controller.show(
+                self.req, self.disabled_type['flavorid'])['flavor']
+
+        self.assertEqual(flavor['name'], self.disabled_type['name'])
+
+        # FIXME(sirp): the disabled field is currently namespaced so that we
+        # don't impact the Openstack API. Eventually this should probably be
+        # made a first-class attribute in the next OSAPI version.
+        self.assert_('OS-FLV-DISABLED:disabled' in flavor)
+
+    def test_show_should_include_disabled_flavor_for_admin(self):
+        self.context.is_admin = True
+
+        flavor = self.controller.show(
+                self.req, self.disabled_type['flavorid'])['flavor']
+
+        self.assertEqual(flavor['name'], self.disabled_type['name'])
+        self.assert_('OS-FLV-DISABLED:disabled' in flavor)