From 17a8e8a68cbe4045a1bc2889d1bf51f2db7ebcca Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Mon, 21 Mar 2016 07:21:28 -0700 Subject: [PATCH] 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 | 4 ++ nova/objects/flavor.py | 39 ++++++++++++++- nova/test.py | 6 +++ nova/tests/functional/db/test_flavor.py | 48 ++++++++++++++----- .../functional/wsgi/test_flavor_manage.py | 14 ++++++ .../openstack/compute/test_flavor_manage.py | 32 +++++-------- .../compute/test_flavors_extra_specs.py | 3 +- nova/tests/unit/conductor/test_conductor.py | 2 + nova/tests/unit/db/test_db_api.py | 20 ++++++++ nova/tests/unit/objects/test_flavor.py | 40 +++++++++------- ...oved-to-api-database-b33489ed3b1b246b.yaml | 14 ++++++ 11 files changed, 168 insertions(+), 54 deletions(-) create mode 100644 releasenotes/notes/flavors-moved-to-api-database-b33489ed3b1b246b.yaml diff --git a/nova/api/openstack/compute/flavor_manage.py b/nova/api/openstack/compute/flavor_manage.py index 8f239f623ae..5dd7420b351 100644 --- a/nova/api/openstack/compute/flavor_manage.py +++ b/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()) diff --git a/nova/objects/flavor.py b/nova/objects/flavor.py index 77f69b624eb..f08b64ec3b6 100644 --- a/nova/objects/flavor.py +++ b/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}) diff --git a/nova/test.py b/nova/test.py index 9169478d14e..89bd0d7b383 100644 --- a/nova/test.py +++ b/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()) diff --git a/nova/tests/functional/db/test_flavor.py b/nova/tests/functional/db/test_flavor.py index 00c8a1e0f57..4724e54d92e 100644 --- a/nova/tests/functional/db/test_flavor.py +++ b/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,12 +80,8 @@ 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() @@ -77,6 +89,7 @@ def test_get_with_no_projects(self): 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,7 +202,7 @@ 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) @@ -194,17 +210,17 @@ 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): diff --git a/nova/tests/functional/wsgi/test_flavor_manage.py b/nova/tests/functional/wsgi/test_flavor_manage.py index 5a2d7d89ab5..b259113592a 100644 --- a/nova/tests/functional/wsgi/test_flavor_manage.py +++ b/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. diff --git a/nova/tests/unit/api/openstack/compute/test_flavor_manage.py b/nova/tests/unit/api/openstack/compute/test_flavor_manage.py index a578d6b6768..666db219937 100644 --- a/nova/tests/unit/api/openstack/compute/test_flavor_manage.py +++ b/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": { diff --git a/nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py b/nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py index 7e5778ed8a6..f7348f8931e 100644 --- a/nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py +++ b/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() diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 3b3dde144f9..19feca2fe73 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/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(), diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 85cd569ddf5..3b00ed8fc1e 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/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 diff --git a/nova/tests/unit/objects/test_flavor.py b/nova/tests/unit/objects/test_flavor.py index df26950ed40..cffad0f2ee4 100644 --- a/nova/tests/unit/objects/test_flavor.py +++ b/nova/tests/unit/objects/test_flavor.py @@ -82,20 +82,20 @@ def _compare(test, db, obj): def test_get_by_id(self): with mock.patch.object(db, 'flavor_get') as get: get.return_value = fake_flavor - flavor = flavor_obj.Flavor.get_by_id(self.context, 1) + flavor = flavor_obj.Flavor.get_by_id(self.context, 100) self._compare(self, fake_flavor, flavor) def test_get_by_name(self): with mock.patch.object(db, 'flavor_get_by_name') as get_by_name: get_by_name.return_value = fake_flavor - flavor = flavor_obj.Flavor.get_by_name(self.context, 'm1.foo') + flavor = flavor_obj.Flavor.get_by_name(self.context, 'm1.legacy') self._compare(self, fake_flavor, flavor) def test_get_by_flavor_id(self): with mock.patch.object(db, 'flavor_get_by_flavor_id') as get_by_id: get_by_id.return_value = fake_flavor flavor = flavor_obj.Flavor.get_by_flavor_id(self.context, - 'm1.foo') + 'm1.legacy') self._compare(self, fake_flavor, flavor) @mock.patch('nova.objects.Flavor._flavor_get_from_db') @@ -334,9 +334,9 @@ def test_save_api(self, mock_update, mock_delete, mock_remove, mock_add): mock_add.assert_called_once_with(self.context, 123, 'project-3') @mock.patch('nova.objects.Flavor._flavor_create') - @mock.patch('nova.db.flavor_extra_specs_delete') - @mock.patch('nova.db.flavor_extra_specs_update_or_create') - def test_save_deleted_extra_specs(self, mock_update, mock_delete, + @mock.patch('nova.objects.Flavor._flavor_extra_specs_del') + @mock.patch('nova.objects.Flavor._flavor_extra_specs_add') + def test_save_deleted_extra_specs(self, mock_add, mock_delete, mock_create): mock_create.return_value = dict(fake_flavor, extra_specs={'key1': 'value1'}) @@ -346,9 +346,8 @@ def test_save_deleted_extra_specs(self, mock_update, mock_delete, flavor.create() flavor.extra_specs = {} flavor.save() - mock_delete.assert_called_once_with(self.context, flavor.flavorid, - 'key1') - self.assertFalse(mock_update.called) + mock_delete.assert_called_once_with(self.context, flavor.id, 'key1') + self.assertFalse(mock_add.called) def test_save_invalid_fields(self): flavor = flavor_obj.Flavor(id=123) @@ -471,18 +470,23 @@ class TestFlavorRemote(test_objects._RemoteTest, _TestFlavor): class _TestFlavorList(object): - def test_get_all_from_db(self): - db_flavors = [ - _TestFlavor._create_api_flavor(self.context), - _TestFlavor._create_api_flavor(self.context, 'm1.bar'), - ] - db_flavors.extend(db_api.flavor_get_all(self.context)) + @mock.patch('nova.db.flavor_get_all') + def test_get_all_from_db(self, mock_get): + # Get a list of the actual flavors in the API DB + api_flavors = flavor_obj._flavor_get_all_from_db(self.context, + False, None, + 'flavorid', 'asc', + None, None) + # Return a fake flavor from the main DB query + db_flavors = [fake_flavor] + mock_get.return_value = db_flavors + flavors = objects.FlavorList.get_all(self.context) - self.assertEqual(len(db_flavors), len(flavors)) + # Make sure we're getting all flavors from the api and main + # db queries + self.assertEqual(len(db_flavors) + len(api_flavors), len(flavors)) def test_get_all_from_db_with_limit(self): - _TestFlavor._create_api_flavor(self.context) - _TestFlavor._create_api_flavor(self.context, 'm1.bar') flavors = objects.FlavorList.get_all(self.context, limit=1) self.assertEqual(1, len(flavors)) diff --git a/releasenotes/notes/flavors-moved-to-api-database-b33489ed3b1b246b.yaml b/releasenotes/notes/flavors-moved-to-api-database-b33489ed3b1b246b.yaml new file mode 100644 index 00000000000..47126fc72fb --- /dev/null +++ b/releasenotes/notes/flavors-moved-to-api-database-b33489ed3b1b246b.yaml @@ -0,0 +1,14 @@ +--- +upgrade: + - Flavors are being moved to the API database for CellsV2. In this + release, the online data migrations will move any flavors you have + in your main database to the API database, retaining all + attributes. Until this is complete, new attempts to create flavors + will return an HTTP 409 to avoid creating flavors in one place that + may conflict with flavors you already have and are yet to be + migrated. + - Note that flavors can no longer be soft-deleted as the API + database does not replicate the legacy soft-delete functionality + from the main database. As such, deleted flavors are not migrated + and the behavior users will experience will be the same as if a + purge of deleted records was performed. \ No newline at end of file