Merge "Convert instancetype.flavorid to string"

14 files changed, 368 insertions, 181 deletions

diff --git a/nova/api/ec2/admin.py b/nova/api/ec2/admin.py
index c1b1fa53a..d9a64506d 100644
--- a/nova/api/ec2/admin.py
+++ b/nova/api/ec2/admin.py
@@ -25,6 +25,7 @@ import netaddr
 import urllib
 
 from nova import compute
+from nova.compute import instance_types
 from nova import db
 from nova import exception
 from nova import flags
@@ -126,8 +127,8 @@ class AdminController(object):
 
     def describe_instance_types(self, context, **_kwargs):
         """Returns all active instance types data (vcpus, memory, etc.)"""
-        inst_types = db.instance_type_get_all(context)
-        inst_type_dicts = [instance_dict(i) for i in inst_types]
+        inst_types = instance_types.get_all_types()
+        inst_type_dicts = [instance_dict(i) for i in inst_types.values()]
         return {'instanceTypeSet': inst_type_dicts}
 
     def describe_user(self, _context, name, **_kwargs):
diff --git a/nova/api/openstack/flavors.py b/nova/api/openstack/flavors.py
index 0727ee258..4c3e860d6 100644
--- a/nova/api/openstack/flavors.py
+++ b/nova/api/openstack/flavors.py
@@ -18,11 +18,12 @@
 import webob
 from lxml import etree
 
-from nova import db
-from nova import exception
 from nova.api.openstack import views
 from nova.api.openstack import wsgi
 from nova.api.openstack import xmlutil
+from nova.compute import instance_types
+from nova import db
+from nova import exception
 
 
 class Controller(object):
@@ -57,17 +58,17 @@ class Controller(object):
                 pass  # ignore bogus values per spec
 
         ctxt = req.environ['nova.context']
-        inst_types = db.api.instance_type_get_all(ctxt, filters=filters)
+        inst_types = instance_types.get_all_types(filters=filters)
         builder = self._get_view_builder(req)
         items = [builder.build(inst_type, is_detail=is_detail)
-                 for inst_type in inst_types]
+                 for inst_type in inst_types.values()]
         return items
 
     def show(self, req, id):
         """Return data about the given flavor id."""
         try:
             ctxt = req.environ['nova.context']
-            flavor = db.api.instance_type_get_by_flavor_id(ctxt, id)
+            flavor = instance_types.get_instance_type_by_flavor_id(id)
         except exception.NotFound:
             return webob.exc.HTTPNotFound()
 
diff --git a/nova/compute/api.py b/nova/compute/api.py
index a39d35006..ae8e95cc5 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -905,7 +905,7 @@ class API(base.Base):
 
     def get_instance_type(self, context, instance_type_id):
         """Get an instance type by instance type id."""
-        return self.db.instance_type_get(context, instance_type_id)
+        return instance_types.get_instance_type(instance_type_id)
 
     def get(self, context, instance_id):
         """Get a single instance with the given instance_id."""
@@ -948,8 +948,8 @@ class API(base.Base):
         filters = {}
 
         def _remap_flavor_filter(flavor_id):
-            instance_type = self.db.instance_type_get_by_flavor_id(
-                    context, flavor_id)
+            instance_type = instance_types.get_instance_type_by_flavor_id(
+                    flavor_id)
             filters['instance_type_id'] = instance_type['id']
 
         def _remap_fixed_ip_filter(fixed_ip):
@@ -1258,8 +1258,8 @@ class API(base.Base):
             LOG.debug(_("flavor_id is None. Assuming migration."))
             new_instance_type = current_instance_type
         else:
-            new_instance_type = self.db.instance_type_get_by_flavor_id(
-                    context, flavor_id)
+            new_instance_type = instance_types.get_instance_type_by_flavor_id(
+                    flavor_id)
 
         current_instance_type_name = current_instance_type['name']
         new_instance_type_name = new_instance_type['name']
diff --git a/nova/compute/instance_types.py b/nova/compute/instance_types.py
index ffbefb24c..b727de2cc 100644
--- a/nova/compute/instance_types.py
+++ b/nova/compute/instance_types.py
@@ -33,66 +33,72 @@ LOG = logging.getLogger('nova.instance_types')
 def create(name, memory, vcpus, local_gb, flavorid, swap=0,
            rxtx_quota=0, rxtx_cap=0):
     """Creates instance types."""
-    for option in [memory, vcpus, local_gb, flavorid]:
+    kwargs = {
+        'memory_mb': memory,
+        'vcpus': vcpus,
+        'local_gb': local_gb,
+        'swap': swap,
+        'rxtx_quota': rxtx_quota,
+        'rxtx_cap': rxtx_cap,
+    }
+
+    # ensure some attributes are integers and greater than or equal to 0
+    for option in kwargs:
         try:
-            int(option)
-        except ValueError:
-            raise exception.InvalidInput(reason=_("create arguments must "
-                                                  "be positive integers"))
-    if (int(memory) <= 0) or (int(vcpus) <= 0) or (int(local_gb) < 0):
-        raise exception.InvalidInput(reason=_("create arguments must "
-                                              "be positive integers"))
+            kwargs[option] = int(kwargs[option])
+            assert kwargs[option] >= 0
+        except (ValueError, AssertionError):
+            msg = _("create arguments must be positive integers")
+            raise exception.InvalidInput(reason=msg)
+
+    # some value are required to be nonzero, not just positive
+    for option in ['memory_mb', 'vcpus']:
+        try:
+            assert kwargs[option] > 0
+        except AssertionError:
+            msg = _("create arguments must be positive integers")
+            raise exception.InvalidInput(reason=msg)
+
+    kwargs['name'] = name
+    kwargs['flavorid'] = flavorid
 
     try:
-        db.instance_type_create(
-                context.get_admin_context(),
-                dict(name=name,
-                    memory_mb=memory,
-                    vcpus=vcpus,
-                    local_gb=local_gb,
-                    flavorid=flavorid,
-                    swap=swap,
-                    rxtx_quota=rxtx_quota,
-                    rxtx_cap=rxtx_cap))
+        return db.instance_type_create(context.get_admin_context(), kwargs)
     except exception.DBError, e:
         LOG.exception(_('DB error: %s') % e)
-        raise exception.ApiError(_("Cannot create instance_type with "
-                                    "name %(name)s and flavorid %(flavorid)s")
-                                    % locals())
+        msg = _("Cannot create instance_type with name %(name)s and "
+                "flavorid %(flavorid)s") % locals()
+        raise exception.ApiError(msg)
 
 
 def destroy(name):
     """Marks instance types as deleted."""
-    if name is None:
-        raise exception.InvalidInstanceType(instance_type=name)
-    else:
-        try:
-            db.instance_type_destroy(context.get_admin_context(), name)
-        except exception.NotFound:
-            LOG.exception(_('Instance type %s not found for deletion') % name)
-            raise exception.ApiError(_("Unknown instance type: %s") % name)
+    try:
+        assert name is not None
+        db.instance_type_destroy(context.get_admin_context(), name)
+    except (AssertionError, exception.NotFound):
+        LOG.exception(_('Instance type %s not found for deletion') % name)
+        raise exception.InstanceTypeNotFoundByName(instance_type_name=name)
 
 
 def purge(name):
     """Removes instance types from database."""
-    if name is None:
-        raise exception.InvalidInstanceType(instance_type=name)
-    else:
-        try:
-            db.instance_type_purge(context.get_admin_context(), name)
-        except exception.NotFound:
-            LOG.exception(_('Instance type %s not found for purge') % name)
-            raise exception.ApiError(_("Unknown instance type: %s") % name)
+    try:
+        assert name is not None
+        db.instance_type_purge(context.get_admin_context(), name)
+    except (AssertionError, exception.NotFound):
+        LOG.exception(_('Instance type %s not found for purge') % name)
+        raise exception.InstanceTypeNotFoundByName(instance_type_name=name)
 
 
-def get_all_types(inactive=0):
+def get_all_types(inactive=0, 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)
+    inst_types = db.instance_type_get_all(ctxt, inactive, filters)
     inst_type_dict = {}
     for inst_type in inst_types:
         inst_type_dict[inst_type['name']] = inst_type
@@ -110,23 +116,27 @@ def get_default_instance_type():
         raise exception.ApiError(_("Unknown instance type: %s") % name)
 
 
-def get_instance_type(id):
+def get_instance_type(instance_type_id):
     """Retrieves single instance type by id."""
-    if id is None:
+    if instance_type_id is None:
         return get_default_instance_type()
+
+    ctxt = context.get_admin_context()
     try:
-        ctxt = context.get_admin_context()
-        return db.instance_type_get(ctxt, id)
+        return db.instance_type_get(ctxt, instance_type_id)
     except exception.DBError:
-        raise exception.ApiError(_("Unknown instance type: %s") % id)
+        msg = _("Unknown instance type: %s") % instance_type_id
+        raise exception.ApiError(msg)
 
 
 def get_instance_type_by_name(name):
     """Retrieves single instance type by name."""
     if name is None:
         return get_default_instance_type()
+
+    ctxt = context.get_admin_context()
+
     try:
-        ctxt = context.get_admin_context()
         return db.instance_type_get_by_name(ctxt, name)
     except exception.DBError:
         raise exception.ApiError(_("Unknown instance type: %s") % name)
@@ -134,10 +144,10 @@ def get_instance_type_by_name(name):
 
 # TODO(termie): flavor-specific code should probably be in the API that uses
 #               flavors.
-def get_instance_type_by_flavor_id(flavor_id):
-    """Retrieve instance type by flavor_id."""
+def get_instance_type_by_flavor_id(flavorid):
+    """Retrieve instance type by flavorid."""
     ctxt = context.get_admin_context()
     try:
-        return db.instance_type_get_by_flavor_id(ctxt, flavor_id)
-    except ValueError:
-        raise exception.FlavorNotFound(flavor_id=flavor_id)
+        return db.instance_type_get_by_flavor_id(ctxt, flavorid)
+    except exception.DBError:
+        raise exception.ApiError(_("Unknown instance type: %s") % flavorid)
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 2ccf44050..2fc2972bf 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -43,23 +43,24 @@ import time
 
 from eventlet import greenthread
 
-import nova.context
 from nova import block_device
+import nova.context
+from nova.compute import instance_types
+from nova.compute import power_state
+from nova.compute import task_states
+from nova.compute.utils import notify_usage_exists
+from nova.compute import vm_states
 from nova import exception
 from nova import flags
 import nova.image
 from nova import log as logging
 from nova import manager
 from nova import network
+from nova.notifier import api as notifier
 from nova import rpc
 from nova import utils
-from nova import volume
-from nova.compute import power_state
-from nova.compute import task_states
-from nova.compute import vm_states
-from nova.notifier import api as notifier
-from nova.compute.utils import notify_usage_exists
 from nova.virt import driver
+from nova import volume
 
 
 FLAGS = flags.FLAGS
@@ -342,8 +343,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                 return
 
             instance_type_id = instance['instance_type_id']
-            instance_type = self.db.instance_type_get(context,
-                    instance_type_id)
+            instance_type = instance_types.get_instance_type(instance_type_id)
             allowed_size_gb = instance_type['local_gb']
 
             # NOTE(jk0): Since libvirt uses local_gb as a secondary drive, we
@@ -985,8 +985,8 @@ class ComputeManager(manager.SchedulerDependentManager):
         instance_ref = self.db.instance_get_by_uuid(context,
                 migration_ref.instance_uuid)
 
-        instance_type = self.db.instance_type_get(context,
-                migration_ref['old_instance_type_id'])
+        old_instance_type = migration_ref['old_instance_type_id']
+        instance_type = instance_types.get_instance_type(old_instance_type)
 
         # Just roll back the record. There's no need to resize down since
         # the 'old' VM already has the preferred attributes
@@ -1029,10 +1029,10 @@ class ComputeManager(manager.SchedulerDependentManager):
             msg = _('Migration error: destination same as source!')
             raise exception.Error(msg)
 
-        old_instance_type = self.db.instance_type_get(context,
-                instance_ref['instance_type_id'])
-        new_instance_type = self.db.instance_type_get(context,
-                instance_type_id)
+        old_instance_type_id = instance_ref['instance_type_id']
+        old_instance_type = instance_types.get_instance_type(
+                old_instance_type_id)
+        new_instance_type = instance_types.get_instance_type(instance_type_id)
 
         migration_ref = self.db.migration_create(context,
                 {'instance_uuid': instance_ref['uuid'],
@@ -1103,10 +1103,11 @@ class ComputeManager(manager.SchedulerDependentManager):
         resize_instance = False
         instance_ref = self.db.instance_get_by_uuid(context,
                 migration_ref.instance_uuid)
-        if migration_ref['old_instance_type_id'] != \
-           migration_ref['new_instance_type_id']:
-            instance_type = self.db.instance_type_get(context,
-                    migration_ref['new_instance_type_id'])
+        old_instance_type_id = migration_ref['old_instance_type_id']
+        new_instance_type_id = migration_ref['new_instance_type_id']
+        if old_instance_type_id != new_instance_type_id:
+            instance_type = instance_types.get_instance_type(
+                    new_instance_type_id)
             self.db.instance_update(context, instance_ref.uuid,
                    dict(instance_type_id=instance_type['id'],
                         memory_mb=instance_type['memory_mb'],
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index cb049652b..370ac3001 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -3368,7 +3368,7 @@ def instance_type_get(context, id):
                     first()
 
     if not inst_type:
-        raise exception.InstanceTypeNotFound(instance_type=id)
+        raise exception.InstanceTypeNotFound(instance_type_id=id)
     else:
         return _dict_with_extra_specs(inst_type)
 
@@ -3388,13 +3388,8 @@ def instance_type_get_by_name(context, name):
 
 
 @require_context
-def instance_type_get_by_flavor_id(context, id):
+def instance_type_get_by_flavor_id(context, flavor_id):
     """Returns a dict describing specific flavor_id"""
-    try:
-        flavor_id = int(id)
-    except ValueError:
-        raise exception.FlavorNotFound(flavor_id=id)
-
     session = get_session()
     inst_type = session.query(models.InstanceTypes).\
                                     options(joinedload('extra_specs')).\
diff --git a/nova/db/sqlalchemy/migrate_repo/versions/055_convert_flavor_id_to_str.py b/nova/db/sqlalchemy/migrate_repo/versions/055_convert_flavor_id_to_str.py
new file mode 100644
index 000000000..082b7c455
--- /dev/null
+++ b/nova/db/sqlalchemy/migrate_repo/versions/055_convert_flavor_id_to_str.py
@@ -0,0 +1,117 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+# All Rights Reserved.
+#
+#    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.
+
+import migrate
+import migrate.changeset
+import sqlalchemy
+
+from nova import log as logging
+
+
+LOG = logging.getLogger('nova.db.sqlalchemy.migrate_repo.versions')
+
+meta = sqlalchemy.MetaData()
+
+
+def _get_table():
+    return sqlalchemy.Table('instance_types', meta, autoload=True)
+
+
+def upgrade(migrate_engine):
+    meta.bind = migrate_engine
+    instance_types = _get_table()
+
+    string_column = sqlalchemy.Column('flavorid_str', sqlalchemy.String(255))
+
+    string_column.create(instance_types)
+
+    try:
+        # NOTE(bcwaldon): This catches a bug with python-migrate
+        # failing to add the unique constraint
+        try:
+            migrate.UniqueConstraint(string_column).create()
+        except migrate.changeset.NotSupportedError:
+            LOG.error("Failed to add unique constraint on flavorid")
+            pass
+
+        # NOTE(bcwaldon): this is a hack to preserve uniqueness constraint
+        # on existing 'name' column
+        try:
+            migrate.UniqueConstraint(instance_types.c.name).create()
+        except Exception:
+            pass
+
+        integer_column = instance_types.c.flavorid
+
+        instance_type_rows = list(instance_types.select().execute())
+        for instance_type in instance_type_rows:
+            flavorid_int = instance_type.flavorid
+            instance_types.update()\
+                          .where(integer_column == flavorid_int)\
+                          .values(flavorid_str=str(flavorid_int))\
+                          .execute()
+    except Exception:
+        string_column.drop()
+        raise
+
+    integer_column.alter(name='flavorid_int')
+    string_column.alter(name='flavorid')
+    integer_column.drop()
+
+
+def downgrade(migrate_engine):
+    meta.bind = migrate_engine
+    instance_types = _get_table()
+
+    integer_column = sqlalchemy.Column('flavorid_int',
+                                       sqlalchemy.Integer())
+
+    integer_column.create(instance_types)
+
+    try:
+        # NOTE(bcwaldon): This catches a bug with python-migrate
+        # failing to add the unique constraint
+        try:
+            migrate.UniqueConstraint(integer_column).create()
+        except migrate.changeset.NotSupportedError:
+            LOG.info("Failed to add unique constraint on flavorid")
+            pass
+
+        string_column = instance_types.c.flavorid
+
+        instance_types_rows = list(instance_types.select().execute())
+        for instance_type in instance_types_rows:
+            flavorid_str = instance_type.flavorid
+            try:
+                flavorid_int = int(instance_type.flavorid)
+            except ValueError:
+                msg = _('Could not cast flavorid to integer: %s. '
+                        'Set flavorid to an integer-like string to downgrade.')
+                LOG.error(msg % instance_type.flavorid)
+                raise
+
+            instance_types.update()\
+                          .where(string_column == flavorid_str)\
+                          .values(flavorid_int=flavorid_int)\
+                          .execute()
+    except Exception:
+        integer_column.drop()
+        raise
+
+    string_column.alter(name='flavorid_str')
+    integer_column.alter(name='flavorid')
+    string_column.drop()
diff --git a/nova/db/sqlalchemy/models.py b/nova/db/sqlalchemy/models.py
index 6240fcff0..e493c78d5 100644
--- a/nova/db/sqlalchemy/models.py
+++ b/nova/db/sqlalchemy/models.py
@@ -303,7 +303,7 @@ class InstanceTypes(BASE, NovaBase):
     memory_mb = Column(Integer)
     vcpus = Column(Integer)
     local_gb = Column(Integer)
-    flavorid = Column(Integer, unique=True)
+    flavorid = Column(String(255), unique=True)
     swap = Column(Integer, nullable=False, default=0)
     rxtx_quota = Column(Integer, nullable=False, default=0)
     rxtx_cap = Column(Integer, nullable=False, default=0)
diff --git a/nova/network/manager.py b/nova/network/manager.py
index 958ddf201..f1070f960 100644
--- a/nova/network/manager.py
+++ b/nova/network/manager.py
@@ -48,10 +48,13 @@ import datetime
 import itertools
 import math
 import netaddr
+import random
 import re
 import socket
 from eventlet import greenpool
 
+from nova.compute import api as compute_api
+from nova.compute import instance_types
 from nova import context
 from nova import db
 from nova import exception
@@ -59,12 +62,10 @@ from nova import flags
 from nova import ipv6
 from nova import log as logging
 from nova import manager
+from nova.network import api as network_api
 from nova import quota
 from nova import utils
 from nova import rpc
-from nova.network import api as network_api
-from nova.compute import api as compute_api
-import random
 
 
 LOG = logging.getLogger("nova.network.manager")
@@ -661,7 +662,7 @@ class NetworkManager(manager.SchedulerDependentManager):
             fixed_ips = []
 
         vifs = self.db.virtual_interface_get_by_instance(context, instance_id)
-        flavor = self.db.instance_type_get(context, instance_type_id)
+        instance_type = instance_types.get_instance_type(instance_type_id)
         network_info = []
         # a vif has an address, instance_id, and network_id
         # it is also joined to the instance and network given by those IDs
@@ -711,7 +712,7 @@ class NetworkManager(manager.SchedulerDependentManager):
                 'broadcast': network['broadcast'],
                 'mac': vif['address'],
                 'vif_uuid': vif['uuid'],
-                'rxtx_cap': flavor['rxtx_cap'],
+                'rxtx_cap': instance_type['rxtx_cap'],
                 'dns': [],
                 'ips': [ip_dict(ip) for ip in network_IPs],
                 'should_create_bridge': self.SHOULD_CREATE_BRIDGE,
diff --git a/nova/tests/api/openstack/test_flavors.py b/nova/tests/api/openstack/test_flavors.py
index 6cea114b1..703009e3c 100644
--- a/nova/tests/api/openstack/test_flavors.py
+++ b/nova/tests/api/openstack/test_flavors.py
@@ -16,14 +16,15 @@
 #    under the License.
 
 import json
-import webob
+
 from lxml import etree
+import webob
 
 from nova.api.openstack import flavors
-import nova.db.api
+from nova.api.openstack import xmlutil
+import nova.compute.instance_types
 from nova import exception
 from nova import test
-from nova.api.openstack import xmlutil
 from nova.tests.api.openstack import fakes
 from nova import wsgi
 
@@ -48,30 +49,33 @@ FAKE_FLAVORS = {
 }
 
 
-def fake_instance_type_get_by_flavor_id(context, flavorid):
+def fake_instance_type_get_by_flavor_id(flavorid):
     return FAKE_FLAVORS['flavor %s' % flavorid]
 
 
-def fake_instance_type_get_all(context, inactive=False, filters=None):
+def fake_instance_type_get_all(inactive=False, filters=None):
     def reject_min(db_attr, filter_attr):
         return filter_attr in filters and\
                int(flavor[db_attr]) < int(filters[filter_attr])
 
     filters = filters or {}
-    for flavor in FAKE_FLAVORS.values():
+    output = {}
+    for (flavor_name, flavor) in FAKE_FLAVORS.items():
         if reject_min('memory_mb', 'min_memory_mb'):
             continue
         elif reject_min('local_gb', 'min_local_gb'):
             continue
 
-        yield flavor
+        output[flavor_name] = flavor
+
+    return output
 
 
-def empty_instance_type_get_all(context, inactive=False, filters=None):
+def empty_instance_type_get_all(inactive=False, filters=None):
     return {}
 
 
-def return_instance_type_not_found(context, flavor_id):
+def return_instance_type_not_found(flavor_id):
     raise exception.InstanceTypeNotFound(flavor_id=flavor_id)
 
 
@@ -80,9 +84,10 @@ class FlavorsTest(test.TestCase):
         super(FlavorsTest, self).setUp()
         fakes.stub_out_networking(self.stubs)
         fakes.stub_out_rate_limiting(self.stubs)
-        self.stubs.Set(nova.db.api, "instance_type_get_all",
+        self.stubs.Set(nova.compute.instance_types, "get_all_types",
                        fake_instance_type_get_all)
-        self.stubs.Set(nova.db.api, "instance_type_get_by_flavor_id",
+        self.stubs.Set(nova.compute.instance_types,
+                       "get_instance_type_by_flavor_id",
                        fake_instance_type_get_by_flavor_id)
 
     def tearDown(self):
@@ -90,7 +95,8 @@ class FlavorsTest(test.TestCase):
         super(FlavorsTest, self).tearDown()
 
     def test_get_flavor_by_invalid_id(self):
-        self.stubs.Set(nova.db.api, "instance_type_get_by_flavor_id",
+        self.stubs.Set(nova.compute.instance_types,
+                       "get_instance_type_by_flavor_id",
                        return_instance_type_not_found)
         req = webob.Request.blank('/v1.1/fake/flavors/asdf')
         res = req.get_response(fakes.wsgi_app())
@@ -216,7 +222,7 @@ class FlavorsTest(test.TestCase):
         self.assertEqual(flavor, expected)
 
     def test_get_empty_flavor_list(self):
-        self.stubs.Set(nova.db.api, "instance_type_get_all",
+        self.stubs.Set(nova.compute.instance_types, "get_all_types",
                        empty_instance_type_get_all)
 
         req = webob.Request.blank('/v1.1/fake/flavors')
diff --git a/nova/tests/api/openstack/test_servers.py b/nova/tests/api/openstack/test_servers.py
index 8ca45297e..f3c61e48e 100644
--- a/nova/tests/api/openstack/test_servers.py
+++ b/nova/tests/api/openstack/test_servers.py
@@ -19,11 +19,11 @@
 import base64
 import datetime
 import json
-from lxml import etree
 import unittest
 import urlparse
 from xml.dom import minidom
 
+from lxml import etree
 import webob
 
 import nova.api.openstack
@@ -40,13 +40,13 @@ import nova.db.api
 from nova.db.sqlalchemy.models import InstanceMetadata
 from nova import exception
 from nova import flags
+import nova.image.fake
+import nova.rpc
 import nova.scheduler.api
 from nova import test
 from nova.tests.api.openstack import common
 from nova.tests.api.openstack import fakes
 from nova import utils
-import nova.image.fake
-import nova.rpc
 
 
 FLAGS = flags.FLAGS
@@ -2615,7 +2615,7 @@ class ServersViewBuilderTest(test.TestCase):
             "hostname": "",
             "host": "",
             "instance_type": {
-               "flavorid": 1,
+               "flavorid": '1',
             },
             "user_data": "",
             "reservation_id": "",
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index d2f0b2193..dafaea54a 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -695,7 +695,7 @@ class ComputeTestCase(test.TestCase):
         inst_ref = db.instance_get(context, instance_id)
         instance_type_ref = db.instance_type_get(context,
                 inst_ref['instance_type_id'])
-        self.assertEqual(instance_type_ref['flavorid'], 1)
+        self.assertEqual(instance_type_ref['flavorid'], '1')
 
         db.instance_update(self.context, instance_id, {'host': 'foo'})
 
@@ -715,7 +715,7 @@ class ComputeTestCase(test.TestCase):
         inst_ref = db.instance_get(context, instance_id)
         instance_type_ref = db.instance_type_get(context,
                 inst_ref['instance_type_id'])
-        self.assertEqual(instance_type_ref['flavorid'], 3)
+        self.assertEqual(instance_type_ref['flavorid'], '3')
 
         # Finally, revert and confirm the old flavor has been applied
         self.compute.revert_resize(context, inst_ref['uuid'],
@@ -726,7 +726,7 @@ class ComputeTestCase(test.TestCase):
         inst_ref = db.instance_get(context, instance_id)
         instance_type_ref = db.instance_type_get(context,
                 inst_ref['instance_type_id'])
-        self.assertEqual(instance_type_ref['flavorid'], 1)
+        self.assertEqual(instance_type_ref['flavorid'], '1')
 
         self.compute.terminate_instance(context, instance_id)
 
diff --git a/nova/tests/test_instance_types.py b/nova/tests/test_instance_types.py
index 1ed34fba3..a3fb8a049 100644
--- a/nova/tests/test_instance_types.py
+++ b/nova/tests/test_instance_types.py
@@ -37,27 +37,18 @@ class InstanceTypeTestCase(test.TestCase):
     def setUp(self):
         super(InstanceTypeTestCase, self).setUp()
         session = get_session()
-        max_flavorid = session.query(models.InstanceTypes).\
-                                     order_by("flavorid desc").\
-                                     first()
-        max_id = session.query(models.InstanceTypes).\
-                                     order_by("id desc").\
-                                     first()
-        self.flavorid = max_flavorid["flavorid"] + 1
-        self.id = max_id["id"] + 1
-        self.name = str(int(time.time()))
-
-    def _nonexistent_flavor_name(self):
-        """return an instance type name not in the DB"""
-        nonexistent_flavor = "sdfsfsdf"
+
+    def _generate_name(self):
+        """return a name not in the DB"""
+        nonexistent_flavor = str(int(time.time()))
         flavors = instance_types.get_all_types()
         while nonexistent_flavor in flavors:
             nonexistent_flavor += "z"
         else:
             return nonexistent_flavor
 
-    def _nonexistent_flavor_id(self):
-        """return an instance type ID not in the DB"""
+    def _generate_flavorid(self):
+        """return a flavorid not in the DB"""
         nonexistent_flavor = 2700
         flavor_ids = [value["id"] for key, value in\
                     instance_types.get_all_types().iteritems()]
@@ -72,95 +63,160 @@ class InstanceTypeTestCase(test.TestCase):
 
     def test_instance_type_create_then_delete(self):
         """Ensure instance types can be created"""
-        starting_inst_list = instance_types.get_all_types()
-        instance_types.create(self.name, 256, 1, 120, self.flavorid)
-        new = instance_types.get_all_types()
-        self.assertNotEqual(len(starting_inst_list),
-                            len(new),
+        name = 'Small Flavor'
+        flavorid = 'flavor1'
+
+        original_list = instance_types.get_all_types()
+
+        # create new type and make sure values stick
+        inst_type = instance_types.create(name, 256, 1, 120, flavorid)
+        inst_type_id = inst_type['id']
+        self.assertEqual(inst_type['flavorid'], flavorid)
+        self.assertEqual(inst_type['name'], name)
+        self.assertEqual(inst_type['memory_mb'], 256)
+        self.assertEqual(inst_type['vcpus'], 1)
+        self.assertEqual(inst_type['local_gb'], 120)
+        self.assertEqual(inst_type['swap'], 0)
+        self.assertEqual(inst_type['rxtx_quota'], 0)
+        self.assertEqual(inst_type['rxtx_cap'], 0)
+
+        # make sure new type shows up in list
+        new_list = instance_types.get_all_types()
+        self.assertNotEqual(len(original_list), len(new_list),
                             'instance type was not created')
-        instance_types.destroy(self.name)
-        self.assertEqual(1,
-                    instance_types.get_instance_type(self.id)["deleted"])
-        self.assertEqual(starting_inst_list, instance_types.get_all_types())
-        instance_types.purge(self.name)
-        self.assertEqual(len(starting_inst_list),
-                         len(instance_types.get_all_types()),
+
+        # destroy instance and make sure deleted flag is set to True
+        instance_types.destroy(name)
+        inst_type = instance_types.get_instance_type(inst_type_id)
+        self.assertEqual(1, inst_type["deleted"])
+
+        # deleted instance should not be in list anymoer
+        new_list = instance_types.get_all_types()
+        self.assertEqual(original_list, new_list)
+
+        # ensure instances are gone after purge
+        instance_types.purge(name)
+        new_list = instance_types.get_all_types()
+        self.assertEqual(original_list, new_list,
                          'instance type not purged')
 
     def test_get_all_instance_types(self):
         """Ensures that all instance types can be retrieved"""
         session = get_session()
-        total_instance_types = session.query(models.InstanceTypes).\
-                                            count()
+        total_instance_types = session.query(models.InstanceTypes).count()
         inst_types = instance_types.get_all_types()
         self.assertEqual(total_instance_types, len(inst_types))
 
     def test_invalid_create_args_should_fail(self):
         """Ensures that instance type creation fails with invalid args"""
-        self.assertRaises(
-                exception.InvalidInput,
-                instance_types.create, self.name, 0, 1, 120, self.flavorid)
-        self.assertRaises(
-                exception.InvalidInput,
-                instance_types.create, self.name, 256, -1, 120, self.flavorid)
-        self.assertRaises(
-                exception.InvalidInput,
-                instance_types.create, self.name, 256, 1, "aa", self.flavorid)
+        invalid_sigs = [
+            (('Zero memory', 0, 1, 10, 'flavor1'), {}),
+            (('Negative memory', -256, 1, 10, 'flavor1'), {}),
+            (('Non-integer memory', 'asdf', 1, 10, 'flavor1'), {}),
+
+            (('Zero vcpus', 256, 0, 10, 'flavor1'), {}),
+            (('Negative vcpus', 256, -1, 10, 'flavor1'), {}),
+            (('Non-integer vcpus', 256, 'a', 10, 'flavor1'), {}),
+
+            (('Negative storage', 256, 1, -1, 'flavor1'), {}),
+            (('Non-integer storage', 256, 1, 'a', 'flavor1'), {}),
+
+            (('Negative swap', 256, 1, 10, 'flavor1'), {'swap': -1}),
+            (('Non-integer swap', 256, 1, 10, 'flavor1'), {'swap': -1}),
+
+            (('Negative rxtx_quota', 256, 1, 10, 'f1'), {'rxtx_quota': -1}),
+            (('Non-integer rxtx_quota', 256, 1, 10, 'f1'), {'rxtx_quota': -1}),
+
+            (('Negative rxtx_cap', 256, 1, 10, 'f1'), {'rxtx_cap': -1}),
+            (('Non-integer rxtx_cap', 256, 1, 10, 'f1'), {'rxtx_cap': 'a'}),
+        ]
+
+        for (args, kwargs) in invalid_sigs:
+            self.assertRaises(exception.InvalidInput,
+                              instance_types.create, *args, **kwargs)
 
     def test_non_existent_inst_type_shouldnt_delete(self):
         """Ensures that instance type creation fails with invalid args"""
-        self.assertRaises(exception.ApiError,
+        self.assertRaises(exception.InstanceTypeNotFoundByName,
                           instance_types.destroy,
-                          self._nonexistent_flavor_name())
+                          'unknown_flavor')
+
+    def test_duplicate_names_fail(self):
+        """Ensures that name duplicates raise ApiError"""
+        name = 'some_name'
+        instance_types.create(name, 256, 1, 120, 'flavor1')
+        self.assertRaises(exception.ApiError,
+                          instance_types.create,
+                          name, 256, 1, 120, 'flavor2')
 
-    def test_repeated_inst_types_should_raise_api_error(self):
-        """Ensures that instance duplicates raises ApiError"""
-        new_name = self.name + "dup"
-        instance_types.create(new_name, 256, 1, 120, self.flavorid + 1)
-        instance_types.destroy(new_name)
-        self.assertRaises(
-                exception.ApiError,
-                instance_types.create, new_name, 256, 1, 120, self.flavorid)
+    def test_duplicate_flavorids_fail(self):
+        """Ensures that flavorid duplicates raise ApiError"""
+        flavorid = 'flavor1'
+        instance_types.create('name one', 256, 1, 120, flavorid)
+        self.assertRaises(exception.ApiError,
+                          instance_types.create,
+                          'name two', 256, 1, 120, flavorid)
 
     def test_will_not_destroy_with_no_name(self):
         """Ensure destroy sad path of no name raises error"""
-        self.assertRaises(exception.ApiError,
-                          instance_types.destroy,
-                          self._nonexistent_flavor_name())
+        self.assertRaises(exception.InstanceTypeNotFoundByName,
+                          instance_types.destroy, None)
 
     def test_will_not_purge_without_name(self):
         """Ensure purge without a name raises error"""
-        self.assertRaises(exception.InvalidInstanceType,
+        self.assertRaises(exception.InstanceTypeNotFoundByName,
                           instance_types.purge, None)
 
     def test_will_not_purge_with_wrong_name(self):
         """Ensure purge without correct name raises error"""
-        self.assertRaises(exception.ApiError,
+        self.assertRaises(exception.InstanceTypeNotFound,
                           instance_types.purge,
-                          self._nonexistent_flavor_name())
+                          'unknown_flavor')
 
     def test_will_not_get_bad_default_instance_type(self):
         """ensures error raised on bad default instance type"""
-        FLAGS.default_instance_type = self._nonexistent_flavor_name()
+        FLAGS.default_instance_type = 'unknown_flavor'
         self.assertRaises(exception.InstanceTypeNotFoundByName,
                           instance_types.get_default_instance_type)
 
-    def test_will_not_get_instance_type_by_name_with_no_name(self):
+    def test_will_get_instance_type_by_id(self):
+        default_instance_type = instance_types.get_default_instance_type()
+        instance_type_id = default_instance_type['id']
+        fetched = instance_types.get_instance_type(instance_type_id)
+        self.assertEqual(default_instance_type, fetched)
+
+    def test_will_not_get_instance_type_by_unknown_id(self):
         """Ensure get by name returns default flavor with no name"""
-        self.assertEqual(instance_types.get_default_instance_type(),
-                              instance_types.get_instance_type_by_name(None))
+        self.assertRaises(exception.InstanceTypeNotFound,
+                         instance_types.get_instance_type, 10000)
 
-    def test_will_not_get_instance_type_with_bad_name(self):
+    def test_will_not_get_instance_type_with_bad_id(self):
         """Ensure get by name returns default flavor with bad name"""
         self.assertRaises(exception.InstanceTypeNotFound,
-                          instance_types.get_instance_type,
-                          self._nonexistent_flavor_name())
+                          instance_types.get_instance_type, 'asdf')
 
-    def test_will_not_get_flavor_by_bad_flavor_id(self):
+    def test_instance_type_get_by_None_name_returns_default(self):
+        """Ensure get by name returns default flavor with no name"""
+        default = instance_types.get_default_instance_type()
+        actual = instance_types.get_instance_type_by_name(None)
+        self.assertEqual(default, actual)
+
+    def test_will_not_get_instance_type_with_bad_name(self):
+        """Ensure get by name returns default flavor with bad name"""
+        self.assertRaises(exception.InstanceTypeNotFoundByName,
+                          instance_types.get_instance_type_by_name, 10000)
+
+    def test_will_not_get_instance_by_unknown_flavor_id(self):
         """Ensure get by flavor raises error with wrong flavorid"""
-        self.assertRaises(exception.InstanceTypeNotFound,
-                          instance_types.get_instance_type_by_name,
-                          self._nonexistent_flavor_id())
+        self.assertRaises(exception.FlavorNotFound,
+                          instance_types.get_instance_type_by_flavor_id,
+                          'unknown_flavor')
+
+    def test_will_get_instance_by_flavor_id(self):
+        default_instance_type = instance_types.get_default_instance_type()
+        flavorid = default_instance_type['flavorid']
+        fetched = instance_types.get_instance_type_by_flavor_id(flavorid)
+        self.assertEqual(default_instance_type, fetched)
 
 
 class InstanceTypeFilteringTest(test.TestCase):
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index e30c0fce1..9a1ceef4e 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -633,8 +633,7 @@ w
         # FIXME(jk0): this was copied directly from compute.manager.py, let's
         # refactor this to a common area
         instance_type_id = instance['instance_type_id']
-        instance_type = db.instance_type_get(context,
-                instance_type_id)
+        instance_type = instance_types.get_instance_type(instance_type_id)
         allowed_size_gb = instance_type['local_gb']
         allowed_size_bytes = allowed_size_gb * 1024 * 1024 * 1024