Block flavor creation until main database is empty

This makes Flavor.create() fail until the main database has had all
of its flavors migrated. Since we want to avoid any overlap or clashes
in integer ids we need to enforce this. The flavor API was poorly
designed in that it exposes this internal id. Luckily, flavor creation
is infrequent and flavor migration is fast.

The flavors in our database are hard-coded in the first migration,
which means we have to basically do the migration during every
unit test in order to avoid having a legacy environment. Since that
leaves us with deleted flavors in the DB (which screws up the DB
archiving tests), this adds a hard_delete flag to the migration to
do an actual purge of those from our database. What a mess.

Related to blueprint flavor-cell-api

Depends-On: I8ab03af9d2f4974f26a7f8487ec978caea957e45
Change-Id: Iea063448f43da1043d16dd521d255dd29a0e1bc5

nova/api/openstack/compute/flavor_manage.py

@@ -19,6 +19,7 @@
 from nova.api import validation
 from nova.compute import flavors
 from nova import exception
+from nova.i18n import _
 
 ALIAS = "os-flavor-manage"
 
@@ -84,6 +85,9 @@ def _create(self, req, body):
         except (exception.FlavorExists,
                 exception.FlavorIdExists) as err:
             raise webob.exc.HTTPConflict(explanation=err.format_message())
+        except exception.ObjectActionError:
+            raise webob.exc.HTTPConflict(explanation=_(
+                'Not all flavors have been migrated to the API database'))
         except exception.FlavorCreateFailed as err:
             raise webob.exc.HTTPInternalServerError(explanation=
                 err.format_message())

nova/objects/flavor.py

@@ -26,6 +26,7 @@
 from nova.db.sqlalchemy import api as db_api
 from nova.db.sqlalchemy.api import require_context
 from nova.db.sqlalchemy import api_models
+from nova.db.sqlalchemy import models as main_models
 from nova import exception
 from nova.i18n import _LW
 from nova import objects
@@ -189,6 +190,16 @@ def _flavor_destroy(context, flavor_id=None, name=None):
     context.session.delete(result)
 
 
+@db_api.main_context_manager.reader
+def _ensure_migrated(context):
+    result = context.session.query(main_models.InstanceTypes).\
+             filter_by(deleted=0).count()
+    if result:
+        LOG.warning(_LW('Main database contains %(count)i unmigrated flavors'),
+                    {'count': result})
+    return result == 0
+
+
 # TODO(berrange): Remove NovaObjectDictCompat
 @base.NovaObjectRegistry.register
 class Flavor(base.NovaPersistentObject, base.NovaObject,
@@ -453,11 +464,24 @@ def remove_access(self, project_id):
     def _flavor_create(context, updates):
         return _flavor_create(context, updates)
 
+    @staticmethod
+    def _ensure_migrated(context):
+        return _ensure_migrated(context)
+
     @base.remotable
     def create(self):
         if self.obj_attr_is_set('id'):
             raise exception.ObjectActionError(action='create',
                                               reason='already created')
+
+        # NOTE(danms): Once we have made it past a point where we know
+        # all flavors have been migrated, we can remove this. Ideally
+        # in Ocata with a blocker migration to be sure.
+        if not self._ensure_migrated(self._context):
+            raise exception.ObjectActionError(
+                action='create',
+                reason='main database still contains flavors')
+
         updates = self.obj_get_changes()
         expected_attrs = []
         for attr in OPTIONAL_FIELDS:
@@ -671,7 +695,15 @@ def _get_main_db_flavor_ids(context, limit):
             limit(limit)]
 
 
-def migrate_flavors(ctxt, count):
+@db_api.main_context_manager.writer
+def _destroy_flavor_hard(context, name):
+    # NOTE(danms): We don't need this imported at runtime, so
+    # keep it separate here
+    from nova.db.sqlalchemy import models
+    context.session.query(models.InstanceTypes).filter_by(name=name).delete()
+
+
+def migrate_flavors(ctxt, count, hard_delete=False):
     main_db_ids = _get_main_db_flavor_ids(ctxt, count)
     if not main_db_ids:
         return 0, 0
@@ -686,7 +718,10 @@ def migrate_flavors(ctxt, count):
                              for field in flavor.fields}
             flavor._flavor_create(ctxt, flavor_values)
             count_hit += 1
-            db.flavor_destroy(ctxt, flavor.name)
+            if hard_delete:
+                _destroy_flavor_hard(ctxt, flavor.name)
+            else:
+                db.flavor_destroy(ctxt, flavor.name)
         except exception.FlavorNotFound:
             LOG.warning(_LW('Flavor id %(id)i disappeared during migration'),
                         {'id': flavor_id})

nova/test.py

@@ -49,6 +49,7 @@
 from nova.network import manager as network_manager
 from nova.network.security_group import openstack_driver
 from nova.objects import base as objects_base
+from nova.objects import flavor as flavor_obj
 from nova.tests import fixtures as nova_fixtures
 from nova.tests.unit import conf_fixture
 from nova.tests.unit import policy_fixture
@@ -213,6 +214,11 @@ def setUp(self):
         if self.USES_DB:
             self.useFixture(nova_fixtures.Database())
             self.useFixture(nova_fixtures.Database(database='api'))
+            # NOTE(danms): Flavors are encoded in our original migration
+            # which means we have no real option other than to migrate them
+            # onlineish every time we build a new database (for now).
+            ctxt = context.get_admin_context()
+            flavor_obj.migrate_flavors(ctxt, 100, hard_delete=True)
         elif not self.USES_DB_SELF:
             self.useFixture(nova_fixtures.DatabasePoisonFixture())
 

nova/tests/functional/db/test_flavor.py

@@ -39,6 +39,16 @@
     }
 
 
+class ForcedFlavor(objects.Flavor):
+    """A Flavor object that lets us create with things still in the main DB.
+
+    This is required for us to be able to test mixed scenarios.
+    """
+    @staticmethod
+    def _ensure_migrated(*args):
+        return True
+
+
 class FlavorObjectTestCase(test.NoDBTestCase):
     USES_DB_SELF = True
 
@@ -48,7 +58,13 @@ def setUp(self):
         self.useFixture(fixtures.Database(database='api'))
         self.context = context.RequestContext('fake-user', 'fake-project')
 
+    def _delete_main_flavors(self):
+        flavors = db_api.flavor_get_all(self.context)
+        for flavor in flavors:
+            db_api.flavor_destroy(self.context, flavor['name'])
+
     def test_create(self):
+        self._delete_main_flavors()
         flavor = objects.Flavor(context=self.context, **fake_api_flavor)
         flavor.create()
         self.assertIn('id', flavor)
@@ -64,19 +80,16 @@ def test_create(self):
                           db.flavor_get_by_flavor_id, self.context,
                           flavor.flavorid)
 
-        # Default flavors will overlap with our id, so just query and make sure
-        # they are different flavors
-        main_flavor = db.flavor_get(self.context, flavor.id)
-        self.assertNotEqual(flavor.name, main_flavor['name'])
-
     def test_get_with_no_projects(self):
+        self._delete_main_flavors()
         fields = dict(fake_api_flavor, projects=[])
         flavor = objects.Flavor(context=self.context, **fields)
         flavor.create()
         flavor = objects.Flavor.get_by_flavor_id(self.context, flavor.flavorid)
         self.assertEqual([], flavor.projects)
 
     def test_get_with_projects_and_specs(self):
+        self._delete_main_flavors()
         flavor = objects.Flavor(context=self.context, **fake_api_flavor)
         flavor.create()
         flavor = objects.Flavor.get_by_id(self.context, flavor.id)
@@ -95,6 +108,7 @@ def _test_query(self, flavor):
         self.assertEqual(flavor.id, flavor2.id)
 
     def test_query_api(self):
+        self._delete_main_flavors()
         flavor = objects.Flavor(context=self.context, **fake_api_flavor)
         flavor.create()
         self._test_query(flavor)
@@ -126,6 +140,7 @@ def _test_save(self, flavor):
         self.assertEqual(set(projects), set(flavor2.projects))
 
     def test_save_api(self):
+        self._delete_main_flavors()
         flavor = objects.Flavor(context=self.context, **fake_api_flavor)
         flavor.create()
         self._test_save(flavor)
@@ -154,6 +169,7 @@ def _test_destroy(self, flavor):
                           flavor.name)
 
     def test_destroy_api(self):
+        self._delete_main_flavors()
         flavor = objects.Flavor(context=self.context, **fake_api_flavor)
         flavor.create()
         self._test_destroy(flavor)
@@ -186,25 +202,25 @@ def test_get_all(self):
 
     def test_get_all_with_some_api_flavors(self):
         expect_len = len(db_api.flavor_get_all(self.context))
-        flavor = objects.Flavor(context=self.context, **fake_api_flavor)
+        flavor = ForcedFlavor(context=self.context, **fake_api_flavor)
         flavor.create()
         self._test_get_all(expect_len + 1)
 
     def test_get_all_with_all_api_flavors(self):
         db_flavors = db_api.flavor_get_all(self.context)
         for flavor in db_flavors:
             db_api.flavor_destroy(self.context, flavor['name'])
-        flavor = objects.Flavor(context=self.context, **fake_api_flavor)
+        flavor = ForcedFlavor(context=self.context, **fake_api_flavor)
         flavor.create()
         self._test_get_all(1)
 
     def test_get_all_with_marker_in_api(self):
         db_flavors = db_api.flavor_get_all(self.context)
         db_flavor_ids = [x['flavorid'] for x in db_flavors]
-        flavor = objects.Flavor(context=self.context, **fake_api_flavor)
+        flavor = ForcedFlavor(context=self.context, **fake_api_flavor)
         flavor.create()
         fake_flavor2 = dict(fake_api_flavor, name='m1.zoo', flavorid='m1.zoo')
-        flavor = objects.Flavor(context=self.context, **fake_flavor2)
+        flavor = ForcedFlavor(context=self.context, **fake_flavor2)
         flavor.create()
         result = self._test_get_all(3, marker='m1.foo', limit=3)
         result_flavorids = [x.flavorid for x in result]
@@ -213,24 +229,30 @@ def test_get_all_with_marker_in_api(self):
     def test_get_all_with_marker_in_main(self):
         db_flavors = db_api.flavor_get_all(self.context)
         db_flavor_ids = [x['flavorid'] for x in db_flavors]
-        flavor = objects.Flavor(context=self.context, **fake_api_flavor)
+        flavor = ForcedFlavor(context=self.context, **fake_api_flavor)
         flavor.create()
         fake_flavor2 = dict(fake_api_flavor, name='m1.zoo', flavorid='m1.zoo')
-        flavor = objects.Flavor(context=self.context, **fake_flavor2)
+        flavor = ForcedFlavor(context=self.context, **fake_flavor2)
         flavor.create()
         result = self._test_get_all(2, marker='3', limit=3)
         result_flavorids = [x.flavorid for x in result]
         self.assertEqual(db_flavor_ids[3:], result_flavorids)
 
     def test_get_all_with_marker_in_neither(self):
-        flavor = objects.Flavor(context=self.context, **fake_api_flavor)
+        flavor = ForcedFlavor(context=self.context, **fake_api_flavor)
         flavor.create()
         fake_flavor2 = dict(fake_api_flavor, name='m1.zoo', flavorid='m1.zoo')
-        flavor = objects.Flavor(context=self.context, **fake_flavor2)
+        flavor = ForcedFlavor(context=self.context, **fake_flavor2)
         flavor.create()
         self.assertRaises(exception.MarkerNotFound,
                           self._test_get_all, 2, marker='noflavoratall')
 
+    def test_create_checks_main_flavors(self):
+        flavor = objects.Flavor(context=self.context, **fake_api_flavor)
+        self.assertRaises(exception.ObjectActionError, flavor.create)
+        self._delete_main_flavors()
+        flavor.create()
+
 
 class FlavorMigrationTestCase(test.NoDBTestCase):
     def setUp(self):

nova/tests/functional/wsgi/test_flavor_manage.py

@@ -17,6 +17,7 @@
 import six
 
 from nova import context
+from nova import db
 from nova import exception as ex
 from nova import objects
 from nova import test
@@ -179,6 +180,19 @@ def test_flavor_manage_deleted(self):
         resp = self.api.api_get('flavors')
         self.assertFlavorNotInList(new_flav['flavor'], resp.body)
 
+    def test_flavor_create_frozen(self):
+        ctx = context.get_admin_context()
+        db.flavor_create(ctx, {
+            'name': 'foo', 'memory_mb': 512, 'vcpus': 1,
+            'root_gb': 1, 'ephemeral_gb': 0, 'flavorid': 'foo',
+            'swap': 0, 'rxtx_factor': 1.0, 'vcpu_weight': 1,
+            'disabled': False, 'is_public': True,
+        })
+        new_flav = {'flavor': rand_flavor()}
+        resp = self.api.api_post('flavors', new_flav,
+                                 check_response_status=False)
+        self.assertEqual(409, resp.status)
+
     def test_flavor_manage_func(self):
         """Basic flavor creation lifecycle testing.
 

nova/tests/unit/api/openstack/compute/test_flavor_manage.py

@@ -71,25 +71,17 @@ def fake_destroy(flavorname):
     pass
 
 
-def fake_create(context, kwargs):
-    newflavor = fake_db_flavor()
-
-    flavorid = kwargs.get('flavorid')
-    if flavorid is None:
-        flavorid = 1234
-
-    newflavor['flavorid'] = flavorid
-    newflavor["name"] = kwargs.get('name')
-    newflavor["memory_mb"] = int(kwargs.get('memory_mb'))
-    newflavor["vcpus"] = int(kwargs.get('vcpus'))
-    newflavor["root_gb"] = int(kwargs.get('root_gb'))
-    newflavor["ephemeral_gb"] = int(kwargs.get('ephemeral_gb'))
-    newflavor["swap"] = kwargs.get('swap')
-    newflavor["rxtx_factor"] = float(kwargs.get('rxtx_factor'))
-    newflavor["is_public"] = bool(kwargs.get('is_public'))
-    newflavor["disabled"] = bool(kwargs.get('disabled'))
-
-    return newflavor
+def fake_create(newflavor):
+    newflavor['flavorid'] = 1234
+    newflavor["name"] = 'test'
+    newflavor["memory_mb"] = 512
+    newflavor["vcpus"] = 2
+    newflavor["root_gb"] = 1
+    newflavor["ephemeral_gb"] = 1
+    newflavor["swap"] = 512
+    newflavor["rxtx_factor"] = 1.0
+    newflavor["is_public"] = True
+    newflavor["disabled"] = False
 
 
 class FlavorManageTestV21(test.NoDBTestCase):
@@ -103,7 +95,7 @@ def setUp(self):
                        "get_flavor_by_flavor_id",
                        fake_get_flavor_by_flavor_id)
         self.stubs.Set(flavors, "destroy", fake_destroy)
-        self.stub_out("nova.objects.flavor._flavor_create", fake_create)
+        self.stub_out("nova.objects.Flavor.create", fake_create)
 
         self.request_body = {
             "flavor": {

nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py

@@ -27,7 +27,8 @@
 from nova.tests.unit.objects import test_flavor
 
 
-def return_create_flavor_extra_specs(context, flavor_id, extra_specs):
+def return_create_flavor_extra_specs(context, flavor_id, extra_specs,
+                                     *args, **kwargs):
     return stub_flavor_extra_specs()
 
 

nova/tests/unit/conductor/test_conductor.py

@@ -405,6 +405,8 @@ def test_cold_migrate_forced_shutdown(self):
     @mock.patch('nova.objects.Instance.refresh')
     def test_build_instances(self, mock_refresh):
         instance_type = flavors.get_default_flavor()
+        # NOTE(danms): Avoid datetime timezone issues with converted flavors
+        instance_type.created_at = None
         instances = [objects.Instance(context=self.context,
                                       id=i,
                                       uuid=uuid.uuid4(),

nova/tests/unit/db/test_db_api.py

@@ -4135,10 +4135,30 @@ def assert_sorted_by_key_both_dir(sort_key):
             assert_sorted_by_key_both_dir(attr)
 
     def test_flavor_get_all_limit(self):
+        flavors = [
+            {'root_gb': 1, 'memory_mb': 100, 'disabled': True,
+             'is_public': False, 'name': 'flavor1', 'flavorid': 'flavor1'},
+            {'root_gb': 100, 'memory_mb': 200, 'disabled': True,
+             'is_public': False, 'name': 'flavor2', 'flavorid': 'flavor2'},
+            {'root_gb': 100, 'memory_mb': 300, 'disabled': True,
+             'is_public': False, 'name': 'flavor3', 'flavorid': 'flavor3'},
+        ]
+        flavors = [self._create_flavor(it) for it in flavors]
+
         limited_flavors = db.flavor_get_all(self.ctxt, limit=2)
         self.assertEqual(2, len(limited_flavors))
 
     def test_flavor_get_all_list_marker(self):
+        flavors = [
+            {'root_gb': 1, 'memory_mb': 100, 'disabled': True,
+             'is_public': False, 'name': 'flavor1', 'flavorid': 'flavor1'},
+            {'root_gb': 100, 'memory_mb': 200, 'disabled': True,
+             'is_public': False, 'name': 'flavor2', 'flavorid': 'flavor2'},
+            {'root_gb': 100, 'memory_mb': 300, 'disabled': True,
+             'is_public': False, 'name': 'flavor3', 'flavorid': 'flavor3'},
+        ]
+        flavors = [self._create_flavor(it) for it in flavors]
+
         all_flavors = db.flavor_get_all(self.ctxt)
 
         # Set the 3rd result as the marker