diff --git a/nova/api/openstack/compute/flavor_manage.py b/nova/api/openstack/compute/flavor_manage.py index 8f239f623a..5dd7420b35 100644 --- a/nova/api/openstack/compute/flavor_manage.py +++ b/nova/api/openstack/compute/flavor_manage.py @@ -19,6 +19,7 @@ from nova.api.openstack import wsgi 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 @@ class FlavorManageController(wsgi.Controller): 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 77f69b624e..f08b64ec3b 100644 --- a/nova/objects/flavor.py +++ b/nova/objects/flavor.py @@ -26,6 +26,7 @@ from nova import db 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 @@ class Flavor(base.NovaPersistentObject, base.NovaObject, 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 9169478d14..89bd0d7b38 100644 --- a/nova/test.py +++ b/nova/test.py @@ -49,6 +49,7 @@ from nova import db 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 @@ class TestCase(testtools.TestCase): 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 00c8a1e0f5..4724e54d92 100644 --- a/nova/tests/functional/db/test_flavor.py +++ b/nova/tests/functional/db/test_flavor.py @@ -39,6 +39,16 @@ fake_api_flavor = { } +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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 @@ class FlavorObjectTestCase(test.NoDBTestCase): 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 5a2d7d89ab..b259113592 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 @@ class FlavorManageFullstack(test.TestCase): 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 a578d6b676..666db21993 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 @@ class FlavorManageTestV21(test.NoDBTestCase): "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 7e5778ed8a..f7348f8931 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.api.openstack import fakes 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 3b3dde144f..19feca2fe7 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -405,6 +405,8 @@ class _BaseTaskTestCase(object): @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 85cd569ddf..3b00ed8fc1 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -4135,10 +4135,30 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase): 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 df26950ed4..cffad0f2ee 100644 --- a/nova/tests/unit/objects/test_flavor.py +++ b/nova/tests/unit/objects/test_flavor.py @@ -82,20 +82,20 @@ class _TestFlavor(object): 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 @@ class _TestFlavor(object): 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 @@ class _TestFlavor(object): 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 0000000000..47126fc72f --- /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