Merge "Adds `disabled` field for instance-types."

12 files changed, 349 insertions, 8 deletions

diff --git a/nova/api/openstack/compute/contrib/flavorextradata.py b/nova/api/openstack/compute/contrib/flavorextradata.py
index 2864ada5e..d50734817 100644
--- a/nova/api/openstack/compute/contrib/flavorextradata.py
+++ b/nova/api/openstack/compute/contrib/flavorextradata.py
@@ -37,7 +37,8 @@ authorize = extensions.soft_extension_authorizer('compute', 'flavorextradata')
 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
diff --git a/nova/api/openstack/compute/flavors.py b/nova/api/openstack/compute/flavors.py
index 23c918924..56b2e18ab 100644
--- a/nova/api/openstack/compute/flavors.py
+++ b/nova/api/openstack/compute/flavors.py
@@ -94,6 +94,11 @@ class Controller(wsgi.Controller):
     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'])
diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py
index 7bf3d59d0..64af7222f 100644
--- a/nova/api/openstack/compute/servers.py
+++ b/nova/api/openstack/compute/servers.py
@@ -704,6 +704,8 @@ class Controller(wsgi.Controller):
                                                 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:
diff --git a/nova/api/openstack/compute/views/flavors.py b/nova/api/openstack/compute/views/flavors.py
index 7406e8066..c299170b0 100644
--- a/nova/api/openstack/compute/views/flavors.py
+++ b/nova/api/openstack/compute/views/flavors.py
@@ -34,7 +34,7 @@ class ViewBuilder(common.ViewBuilder):
         }
 
     def show(self, request, flavor):
-        return {
+        flavor_dict = {
             "flavor": {
                 "id": flavor["flavorid"],
                 "name": flavor["name"],
@@ -49,6 +49,14 @@ class ViewBuilder(common.ViewBuilder):
             },
         }
 
+        # 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)
diff --git a/nova/compute/api.py b/nova/compute/api.py
index c49604daf..d86e185eb 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -356,6 +356,10 @@ class API(base.Base):
 
         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 @@ class API(base.Base):
         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'])
 
diff --git a/nova/compute/instance_types.py b/nova/compute/instance_types.py
index 66c73c624..69eef7f83 100644
--- a/nova/compute/instance_types.py
+++ b/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
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 0881af62f..d08463c33 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/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]
diff --git a/nova/db/sqlalchemy/migrate_repo/versions/099_add_disabled_instance_types.py b/nova/db/sqlalchemy/migrate_repo/versions/099_add_disabled_instance_types.py
new file mode 100644
index 000000000..166ca98cb
--- /dev/null
+++ b/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)
diff --git a/nova/db/sqlalchemy/models.py b/nova/db/sqlalchemy/models.py
index 452816bb7..df19109d3 100644
--- a/nova/db/sqlalchemy/models.py
+++ b/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),
diff --git a/nova/test.py b/nova/test.py
index dcde09b86..a2edeb51a 100644
--- a/nova/test.py
+++ b/nova/test.py
@@ -291,3 +291,30 @@ class TestCase(unittest.TestCase):
             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
diff --git a/nova/tests/api/openstack/compute/test_flavors.py b/nova/tests/api/openstack/compute/test_flavors.py
index 65474e3b0..d8e87d1c2 100644
--- a/nova/tests/api/openstack/compute/test_flavors.py
+++ b/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 @@ import urlparse
 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 @@ FAKE_FLAVORS = {
         "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 @@ class FlavorsTest(test.TestCase):
         expected = {
             "flavor": {
                 "id": "1",
+                "OS-FLV-DISABLED:disabled": False,
                 "name": "flavor 1",
                 "ram": "256",
                 "disk": "10",
@@ -138,6 +143,7 @@ class FlavorsTest(test.TestCase):
         expected = {
             "flavor": {
                 "id": "1",
+                "OS-FLV-DISABLED:disabled": False,
                 "name": "flavor 1",
                 "ram": "256",
                 "disk": "10",
@@ -308,6 +314,7 @@ class FlavorsTest(test.TestCase):
             "flavors": [
                 {
                     "id": "1",
+                    "OS-FLV-DISABLED:disabled": False,
                     "name": "flavor 1",
                     "ram": "256",
                     "disk": "10",
@@ -327,6 +334,7 @@ class FlavorsTest(test.TestCase):
                 },
                 {
                     "id": "2",
+                    "OS-FLV-DISABLED:disabled": False,
                     "name": "flavor 2",
                     "ram": "512",
                     "disk": "20",
@@ -428,6 +436,7 @@ class FlavorsTest(test.TestCase):
             "flavors": [
                 {
                     "id": "2",
+                    "OS-FLV-DISABLED:disabled": False,
                     "name": "flavor 2",
                     "ram": "512",
                     "disk": "20",
@@ -703,3 +712,98 @@ class FlavorsXMLSerializationTest(test.TestCase):
         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)
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 1a5f2f837..3ed9af763 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -4202,3 +4202,130 @@ class KeypairAPITestCase(BaseTestCase):
         self.assertRaises(exception.KeypairLimitExceeded,
                           self.keypair_api.import_key_pair,
                           self.ctxt, self.ctxt.user_id, 'foo', self.pub_key)
+
+
+class DisabledInstanceTypesTestCase(BaseTestCase):
+    """
+    Some instance-types are marked 'disabled' which means that they will not
+    show up in customer-facing listings. We do, however, want those
+    instance-types to be availble for emergency migrations and for rebuilding
+    of existing instances.
+
+    One legitimate use of the 'disabled' field would be when phasing out a
+    particular instance-type. We still want customers to be able to use an
+    instance that of the old type, and we want Ops to be able perform
+    migrations against it, but we *don't* want customers building new slices
+    with ths phased-out instance-type.
+    """
+    def setUp(self):
+        super(DisabledInstanceTypesTestCase, self).setUp()
+        self.compute_api = compute.API()
+        self.inst_type = instance_types.get_default_instance_type()
+
+    def test_can_build_instance_from_visible_instance_type(self):
+        self.inst_type['disabled'] = False
+
+        self.assertNotRaises(exception.InstanceTypeNotFound,
+            self.compute_api.create, self.context, self.inst_type, None,
+            exc_msg="Visible instance-types can be built from")
+
+    def test_cannot_build_instance_from_disabled_instance_type(self):
+        self.inst_type['disabled'] = True
+        self.assertRaises(exception.InstanceTypeNotFound,
+            self.compute_api.create, self.context, self.inst_type, None)
+
+    def test_can_rebuild_instance_from_visible_instance_type(self):
+        instance = self._create_fake_instance()
+        image_href = None
+        admin_password = 'blah'
+
+        instance['instance_type']['disabled'] = True
+
+        # Assert no errors were raised
+        self.assertNotRaises(None,
+            self.compute_api.rebuild, self.context, instance, image_href,
+            admin_password,
+            exc_msg="Visible instance-types can be rebuilt from")
+
+    def test_can_rebuild_instance_from_disabled_instance_type(self):
+        """
+        A rebuild or a restore should only change the 'image',
+        not the 'instance_type'. Therefore, should be allowed even
+        when the slice is on disabled type already.
+        """
+        instance = self._create_fake_instance()
+        image_href = None
+        admin_password = 'blah'
+
+        instance['instance_type']['disabled'] = True
+
+        # Assert no errors were raised
+        self.assertNotRaises(None,
+            self.compute_api.rebuild, self.context, instance, image_href,
+            admin_password,
+            exc_msg="Disabled instance-types can be rebuilt from")
+
+    def test_can_resize_to_visible_instance_type(self):
+        instance = self._create_fake_instance()
+        orig_get_instance_type_by_flavor_id =\
+                instance_types.get_instance_type_by_flavor_id
+
+        def fake_get_instance_type_by_flavor_id(flavor_id):
+            instance_type = orig_get_instance_type_by_flavor_id(flavor_id)
+            instance_type['disabled'] = False
+            return instance_type
+
+        self.stubs.Set(instance_types, 'get_instance_type_by_flavor_id',
+                       fake_get_instance_type_by_flavor_id)
+
+        # FIXME(sirp): for legacy this raises FlavorNotFound instead of
+        # InstanceTypeNot; we should eventually make it raise
+        # InstanceTypeNotFound for consistency.
+        self.assertNotRaises(exception.FlavorNotFound,
+            self.compute_api.resize, self.context, instance, '4',
+            exc_msg="Visible flavors can be resized to")
+
+    def test_cannot_resize_to_disabled_instance_type(self):
+        instance = self._create_fake_instance()
+        orig_get_instance_type_by_flavor_id = \
+                instance_types.get_instance_type_by_flavor_id
+
+        def fake_get_instance_type_by_flavor_id(flavor_id):
+            instance_type = orig_get_instance_type_by_flavor_id(flavor_id)
+            instance_type['disabled'] = True
+            return instance_type
+
+        self.stubs.Set(instance_types, 'get_instance_type_by_flavor_id',
+                       fake_get_instance_type_by_flavor_id)
+
+        # FIXME(sirp): for legacy this raises FlavorNotFound instead of
+        # InstanceTypeNot; we should eventually make it raise
+        # InstanceTypeNotFound for consistency.
+        self.assertRaises(exception.FlavorNotFound,
+            self.compute_api.resize, self.context, instance, '4')
+
+    def test_can_migrate_to_visible_instance_type(self):
+        instance = self._create_fake_instance()
+        instance['instance_type']['disabled'] = False
+
+        # FIXME(sirp): for legacy this raises FlavorNotFound instead of
+        # InstanceTypeNot; we should eventually make it raise
+        # InstanceTypeNotFound for consistency.
+        self.assertNotRaises(exception.FlavorNotFound,
+            self.compute_api.resize, self.context, instance, None,
+            exc_msg="Visible flavors can be migrated to")
+
+    def test_can_migrate_to_disabled_instance_type(self):
+        """
+        We don't want to require a customers instance-type to change when ops
+        is migrating a failed server.
+        """
+        instance = self._create_fake_instance()
+        instance['instance_type']['disabled'] = True
+
+        # FIXME(sirp): for legacy this raises FlavorNotFound instead of
+        # InstanceTypeNot; we should eventually make it raise
+        # InstanceTypeNotFound for consistency.
+        self.assertNotRaises(exception.FlavorNotFound,
+            self.compute_api.resize, self.context, instance, None,
+            exc_msg="Disabled flavors can be migrated to")