From 98a05bc637e4c2e485d5aa5b62945102ea71d08b Mon Sep 17 00:00:00 2001 From: Andrew Laski Date: Thu, 14 Apr 2016 11:37:02 -0400 Subject: [PATCH] Drop fields from BuildRequest object and model Now that there's a serialized instance object on the BuildRequest object and in the build_requests table a lot of fields/columns are no longer necessary. To prepare for removal of the columns from the db at a later time references to them have been removed. This starts a deprecation timer so that they can be removed in Ocata at the earliest. Note that the BuildRequest object was never passed over the wire, and is just created and immediately destroyed at this point so there is no need to handle backwards compatibility for these changes. Change-Id: Ie86ba66d5f73c153b68bef83661d4c8bdb0ae9dc --- nova/compute/api.py | 41 ++------------ nova/db/sqlalchemy/api_models.py | 33 +++-------- nova/objects/build_request.py | 56 +------------------ .../functional/db/api/test_migrations.py | 17 ++++++ .../tests/functional/db/test_build_request.py | 32 +++-------- nova/tests/unit/compute/test_compute_api.py | 42 +++----------- nova/tests/unit/fake_build_request.py | 49 +--------------- nova/tests/unit/objects/test_build_request.py | 13 +---- nova/tests/unit/objects/test_objects.py | 2 +- 9 files changed, 49 insertions(+), 236 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 2c3d495dd6..90593de69b 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -912,40 +912,6 @@ class API(base.Base): # by the network quotas return base_options, max_network_count, key_pair - def _create_build_request(self, context, instance, base_options, - request_spec, security_groups, num_instances, index): - # Store the BuildRequest that will help populate an instance - # object for a list/show request - info_cache = objects.InstanceInfoCache() - info_cache.instance_uuid = instance.uuid - info_cache.network_info = network_model.NetworkInfo() - # NOTE: base_options['config_drive'] is either True or '' due - # to how it's represented in the instances table in the db. - # BuildRequest needs a boolean. - bool_config_drive = strutils.bool_from_string( - base_options['config_drive'], default=False) - build_request = objects.BuildRequest(context, - instance=instance, - instance_uuid=instance.uuid, - request_spec=request_spec, - project_id=context.project_id, - user_id=context.user_id, - display_name=instance.display_name, - instance_metadata=base_options['metadata'], - progress=0, - vm_state=vm_states.BUILDING, - task_state=task_states.SCHEDULING, - image_ref=base_options['image_ref'], - access_ip_v4=base_options['access_ip_v4'], - access_ip_v6=base_options['access_ip_v6'], - info_cache=info_cache, - security_groups=security_groups, - config_drive=bool_config_drive, - key_name=base_options['key_name'], - locked_by=None) - build_request.create() - return build_request - def _provision_instances(self, context, instance_type, min_count, max_count, base_options, boot_meta, security_groups, block_device_mapping, shutdown_terminate, @@ -984,9 +950,10 @@ class API(base.Base): block_device_mapping, num_instances, i, shutdown_terminate, create_instance=False) - build_request = self._create_build_request(context, - instance, base_options, req_spec, security_groups, - num_instances, i) + build_request = objects.BuildRequest(context, + instance=instance, instance_uuid=instance.uuid, + project_id=instance.project_id) + build_request.create() # Create an instance_mapping. The null cell_mapping indicates # that the instance doesn't yet exist in a cell, and lookups # for it need to instead look for the RequestSpec. diff --git a/nova/db/sqlalchemy/api_models.py b/nova/db/sqlalchemy/api_models.py index ec9b192ef5..413e4f2ac1 100644 --- a/nova/db/sqlalchemy/api_models.py +++ b/nova/db/sqlalchemy/api_models.py @@ -26,8 +26,6 @@ from sqlalchemy import schema from sqlalchemy import String from sqlalchemy import Text -from nova.db.sqlalchemy import types - class _NovaAPIBase(models.ModelBase, models.TimestampMixin): pass @@ -100,11 +98,6 @@ class RequestSpec(API_BASE): id = Column(Integer, primary_key=True) instance_uuid = Column(String(36), nullable=False) spec = Column(Text, nullable=False) - build_request = orm.relationship('BuildRequest', - back_populates='request_spec', - uselist=False, - primaryjoin=( - 'RequestSpec.id == BuildRequest.request_spec_id')) class Flavors(API_BASE): @@ -175,28 +168,16 @@ class BuildRequest(API_BASE): ) id = Column(Integer, primary_key=True) - request_spec_id = Column(Integer, ForeignKey('request_specs.id')) - request_spec = orm.relationship(RequestSpec, - foreign_keys=request_spec_id, - back_populates='build_request', - primaryjoin=request_spec_id == RequestSpec.id) instance_uuid = Column(String(36)) project_id = Column(String(255), nullable=False) - user_id = Column(String(255)) - display_name = Column(String(255)) - instance_metadata = Column(Text) - progress = Column(Integer) - vm_state = Column(String(255)) - task_state = Column(String(255)) - image_ref = Column(String(255)) - access_ip_v4 = Column(types.IPAddress()) - access_ip_v6 = Column(types.IPAddress()) - info_cache = Column(Text) - security_groups = Column(Text) - config_drive = Column(Boolean, default=False) - key_name = Column(String(255)) - locked_by = Column(Enum('owner', 'admin')) instance = Column(Text) + # TODO(alaski): Drop these from the db in Ocata + # columns_to_drop = ['request_spec_id', 'user_id', 'display_name', + # 'instance_metadata', 'progress', 'vm_state', 'task_state', + # 'image_ref', 'access_ip_v4', 'access_ip_v6', 'info_cache', + # 'security_groups', 'config_drive', 'key_name', 'locked_by', + # 'reservation_id', 'launch_index', 'hostname', 'kernel_id', + # 'ramdisk_id', 'root_device_name', 'user_data'] class KeyPair(API_BASE): diff --git a/nova/objects/build_request.py b/nova/objects/build_request.py index d8b1456ff7..0289fe9574 100644 --- a/nova/objects/build_request.py +++ b/nova/objects/build_request.py @@ -15,7 +15,6 @@ from oslo_log import log as logging from oslo_serialization import jsonutils from oslo_versionedobjects import exception as ovoo_exc import six -from sqlalchemy.orm import joinedload from nova.db.sqlalchemy import api as db from nova.db.sqlalchemy import api_models @@ -28,10 +27,6 @@ from nova.objects import fields CONF = cfg.CONF LOG = logging.getLogger(__name__) -OBJECT_FIELDS = ['info_cache', 'security_groups', 'instance'] -JSON_FIELDS = ['instance_metadata'] -IP_FIELDS = ['access_ip_v4', 'access_ip_v6'] - @base.NovaObjectRegistry.register class BuildRequest(base.NovaObject): @@ -42,43 +37,14 @@ class BuildRequest(base.NovaObject): 'id': fields.IntegerField(), 'instance_uuid': fields.UUIDField(), 'project_id': fields.StringField(), - 'user_id': fields.StringField(), - 'display_name': fields.StringField(nullable=True), - 'instance_metadata': fields.DictOfStringsField(nullable=True), - 'progress': fields.IntegerField(nullable=True), - 'vm_state': fields.StringField(nullable=True), - 'task_state': fields.StringField(nullable=True), - 'image_ref': fields.StringField(nullable=True), - 'access_ip_v4': fields.IPV4AddressField(nullable=True), - 'access_ip_v6': fields.IPV6AddressField(nullable=True), - 'info_cache': fields.ObjectField('InstanceInfoCache', nullable=True), - 'security_groups': fields.ObjectField('SecurityGroupList'), - 'config_drive': fields.BooleanField(default=False), - 'key_name': fields.StringField(nullable=True), - 'locked_by': fields.EnumField(['owner', 'admin'], nullable=True), - 'request_spec': fields.ObjectField('RequestSpec'), 'instance': fields.ObjectField('Instance'), # NOTE(alaski): Normally these would come from the NovaPersistentObject # mixin but they're being set explicitly because we only need # created_at/updated_at. There is no soft delete for this object. - # These fields should be carried over to the instance when it is - # scheduled and created in a cell database. 'created_at': fields.DateTimeField(nullable=True), 'updated_at': fields.DateTimeField(nullable=True), } - def _load_request_spec(self, db_spec): - self.request_spec = objects.RequestSpec._from_db_object(self._context, - objects.RequestSpec(), db_spec) - - def _load_info_cache(self, db_info_cache): - self.info_cache = objects.InstanceInfoCache.obj_from_primitive( - jsonutils.loads(db_info_cache)) - - def _load_security_groups(self, db_sec_group): - self.security_groups = objects.SecurityGroupList.obj_from_primitive( - jsonutils.loads(db_sec_group)) - def _load_instance(self, db_instance): # NOTE(alaski): Be very careful with instance loading because it # changes more than most objects. @@ -115,8 +81,6 @@ class BuildRequest(base.NovaObject): getattr(req, '_load_%s' % key)(db_req[key]) except AttributeError: LOG.exception(_LE('No load handler for %s'), key) - elif key in JSON_FIELDS and db_req[key] is not None: - setattr(req, key, jsonutils.loads(db_req[key])) else: setattr(req, key, db_req[key]) req.obj_reset_changes() @@ -126,9 +90,8 @@ class BuildRequest(base.NovaObject): @staticmethod @db.api_context_manager.reader def _get_by_instance_uuid_from_db(context, instance_uuid): - db_req = (context.session.query(api_models.BuildRequest) - .options(joinedload('request_spec')) - .filter_by(instance_uuid=instance_uuid)).first() + db_req = context.session.query(api_models.BuildRequest).filter_by( + instance_uuid=instance_uuid).first() if not db_req: raise exception.BuildRequestNotFound(uuid=instance_uuid) return db_req @@ -144,26 +107,13 @@ class BuildRequest(base.NovaObject): db_req = api_models.BuildRequest() db_req.update(updates) db_req.save(context.session) - # NOTE: This is done because a later access will trigger a lazy load - # outside of the db session so it will fail. We don't lazy load - # request_spec on the object later because we never need a BuildRequest - # without the RequestSpec. - db_req.request_spec return db_req def _get_update_primitives(self): updates = self.obj_get_changes() for key, value in six.iteritems(updates): - if key in OBJECT_FIELDS and value is not None: + if isinstance(self.fields[key], fields.ObjectField): updates[key] = jsonutils.dumps(value.obj_to_primitive()) - elif key in JSON_FIELDS and value is not None: - updates[key] = jsonutils.dumps(value) - elif key in IP_FIELDS and value is not None: - # These are stored as a string in the db and must be converted - updates[key] = str(value) - req_spec_obj = updates.pop('request_spec', None) - if req_spec_obj: - updates['request_spec_id'] = req_spec_obj.id return updates @base.remotable diff --git a/nova/tests/functional/db/api/test_migrations.py b/nova/tests/functional/db/api/test_migrations.py index 657fc6732d..45e8705afb 100644 --- a/nova/tests/functional/db/api/test_migrations.py +++ b/nova/tests/functional/db/api/test_migrations.py @@ -82,6 +82,17 @@ class NovaAPIModelsSync(test_migrations.ModelsMigrationsSync): # the database. They will be removed from the model at a later time. fkey_whitelist = {'build_requests': ['request_spec_id']} + # Define a whitelist of columns that will be removed from the + # DB at a later release and aren't on a model anymore. + + column_whitelist = { + 'build_requests': ['vm_state', 'instance_metadata', + 'display_name', 'access_ip_v6', 'access_ip_v4', 'key_name', + 'locked_by', 'image_ref', 'progress', 'request_spec_id', + 'info_cache', 'user_id', 'task_state', 'security_groups', + 'config_drive'] + } + for element in diff: if isinstance(element, list): # modify_nullable is a list @@ -96,6 +107,12 @@ class NovaAPIModelsSync(test_migrations.ModelsMigrationsSync): if (tablename in fkey_whitelist and column_keys == fkey_whitelist[tablename]): continue + elif element[0] == 'remove_column': + tablename = element[2] + column = element[3] + if (tablename in column_whitelist and + column.name in column_whitelist[tablename]): + continue new_diff.append(element) return new_diff diff --git a/nova/tests/functional/db/test_build_request.py b/nova/tests/functional/db/test_build_request.py index d307832a69..c9a424191f 100644 --- a/nova/tests/functional/db/test_build_request.py +++ b/nova/tests/functional/db/test_build_request.py @@ -22,7 +22,6 @@ from nova.objects import build_request from nova import test from nova.tests import fixtures from nova.tests.unit import fake_build_request -from nova.tests.unit import fake_request_spec from nova.tests.unit.objects import test_objects @@ -40,13 +39,8 @@ class BuildRequestTestCase(test.NoDBTestCase): self.project_id = 'fake-project' def _create_req(self): - req_spec = fake_request_spec.fake_spec_obj(remove_id=True) - req_spec.instance_uuid = self.instance_uuid - req_spec.create() - args = fake_build_request.fake_db_req( - request_spec_id=req_spec.id) + args = fake_build_request.fake_db_req() args.pop('id', None) - args.pop('request_spec', None) args['instance_uuid'] = self.instance_uuid args['project_id'] = self.project_id return build_request.BuildRequest._from_db_object(self.context, @@ -63,8 +57,8 @@ class BuildRequestTestCase(test.NoDBTestCase): db_req = self.build_req_obj._get_by_instance_uuid_from_db(self.context, self.instance_uuid) - flavor_comp = functools.partial(test_objects.compare_obj, self, - allow_missing=['deleted', 'deleted_at', 'created_at', + obj_comp = functools.partial(objects.base.obj_equal_prims, + ignore=['deleted', 'deleted_at', 'created_at', 'updated_at']) def date_comp(db_val, obj_val): @@ -78,20 +72,7 @@ class BuildRequestTestCase(test.NoDBTestCase): for key in self.build_req_obj.fields.keys(): expected = getattr(req, key) db_value = db_req[key] - if key == 'request_spec': - # NOTE: The object and db value can't be compared directly as - # objects, so serialize them to a comparable form. - db_value = jsonutils.dumps(objects.RequestSpec._from_db_object( - self.context, objects.RequestSpec(), - db_value).obj_to_primitive()) - expected = jsonutils.dumps(expected.obj_to_primitive()) - elif key in build_request.OBJECT_FIELDS: - expected = jsonutils.dumps(expected.obj_to_primitive()) - elif key in build_request.JSON_FIELDS: - expected = jsonutils.dumps(expected) - elif key in build_request.IP_FIELDS: - expected = str(expected) - elif key in ['created_at', 'updated_at']: + if key in ['created_at', 'updated_at']: # Objects store tz aware datetimes but the db does not. expected = expected.replace(tzinfo=None) elif key == 'instance': @@ -104,7 +85,8 @@ class BuildRequestTestCase(test.NoDBTestCase): 'ec2_ids', 'migration_context', 'metadata', 'vcpu_model', 'services', 'system_metadata', 'tags', 'fault'], - comparators={'flavor': flavor_comp, - 'created_at': date_comp}) + comparators={'flavor': obj_comp, + 'created_at': date_comp, + 'keypairs': obj_comp}) continue self.assertEqual(expected, db_value) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index c5f533a7c5..76123a2b98 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -3106,19 +3106,19 @@ class _ComputeAPIUnitTestMixIn(object): do_test() @mock.patch('nova.objects.RequestSpec.from_components') + @mock.patch('nova.objects.BuildRequest') @mock.patch('nova.objects.Instance') @mock.patch('nova.objects.InstanceMapping.create') def test_provision_instances_with_keypair(self, mock_im, mock_instance, - mock_rs): + mock_br, mock_rs): fake_keypair = objects.KeyPair(name='test') @mock.patch.object(self.compute_api, '_check_num_instances_quota') @mock.patch.object(self.compute_api, 'security_group_api') @mock.patch.object(self.compute_api, 'create_db_entry_for_new_instance') - @mock.patch.object(self.compute_api, '_create_build_request') @mock.patch.object(self.compute_api, '_create_block_device_mapping') - def do_test(mock_cbdm, mock_cbr, mock_cdb, mock_sg, mock_cniq): + def do_test(mock_cbdm, mock_cdb, mock_sg, mock_cniq): mock_cniq.return_value = 1, mock.MagicMock() self.compute_api._provision_instances(self.context, mock.sentinel.flavor, @@ -3163,6 +3163,8 @@ class _ComputeAPIUnitTestMixIn(object): req_spec_mock = mock.MagicMock() mock_req_spec_from_components.return_value = req_spec_mock inst_mocks = [mock.MagicMock() for i in range(max_count)] + for inst_mock in inst_mocks: + inst_mock.project_id = 'fake-project' mock_inst.side_effect = inst_mocks build_req_mocks = [mock.MagicMock() for i in range(max_count)] mock_build_req.side_effect = build_req_mocks @@ -3222,41 +3224,11 @@ class _ComputeAPIUnitTestMixIn(object): mock.call(ctxt, instance=instances[0], instance_uuid=instances[0].uuid, - request_spec=req_spec_mock, - project_id=ctxt.project_id, - user_id=ctxt.user_id, - display_name=instances[0].display_name, - instance_metadata=base_options['metadata'], - progress=0, - vm_state=vm_states.BUILDING, - task_state=task_states.SCHEDULING, - image_ref=base_options['image_ref'], - access_ip_v4=base_options['access_ip_v4'], - access_ip_v6=base_options['access_ip_v6'], - info_cache=mock.ANY, - security_groups=mock.ANY, - config_drive=False, - key_name=base_options['config_drive'], - locked_by=None), + project_id=instances[0].project_id), mock.call(ctxt, instance=instances[1], instance_uuid=instances[1].uuid, - request_spec=req_spec_mock, - project_id=ctxt.project_id, - user_id=ctxt.user_id, - display_name=instances[1].display_name, - instance_metadata=base_options['metadata'], - progress=0, - vm_state=vm_states.BUILDING, - task_state=task_states.SCHEDULING, - image_ref=base_options['image_ref'], - access_ip_v4=base_options['access_ip_v4'], - access_ip_v6=base_options['access_ip_v6'], - info_cache=mock.ANY, - security_groups=mock.ANY, - config_drive=False, - key_name=base_options['config_drive'], - locked_by=None), + project_id=instances[1].project_id), ] mock_build_req.assert_has_calls(build_req_calls) for build_req_mock in build_req_mocks: diff --git a/nova/tests/unit/fake_build_request.py b/nova/tests/unit/fake_build_request.py index b7e2a68f4e..534ad69884 100644 --- a/nova/tests/unit/fake_build_request.py +++ b/nova/tests/unit/fake_build_request.py @@ -15,56 +15,21 @@ import datetime from oslo_serialization import jsonutils from oslo_utils import uuidutils -from nova.compute import task_states -from nova.compute import vm_states from nova import context -from nova.network import model as network_model from nova import objects from nova.objects import fields from nova.tests.unit import fake_instance -from nova.tests.unit import fake_request_spec - - -def _req_spec_to_db_format(req_spec): - db_spec = {'spec': jsonutils.dumps(req_spec.obj_to_primitive()), - 'id': req_spec.id, - 'instance_uuid': req_spec.instance_uuid, - } - return db_spec def fake_db_req(**updates): ctxt = context.RequestContext('fake-user', 'fake-project') instance_uuid = uuidutils.generate_uuid() - info_cache = objects.InstanceInfoCache() - info_cache.instance_uuid = instance_uuid - info_cache.network_info = network_model.NetworkInfo() - req_spec = fake_request_spec.fake_spec_obj( - context.RequestContext('fake-user', 'fake-project')) - req_spec.id = 42 - req_spec.obj_reset_changes() instance = fake_instance.fake_instance_obj(ctxt, objects.Instance, uuid=instance_uuid) db_build_request = { 'id': 1, 'project_id': 'fake-project', 'instance_uuid': instance_uuid, - 'user_id': 'fake-user', - 'display_name': '', - 'instance_metadata': jsonutils.dumps({'foo': 'bar'}), - 'progress': 0, - 'vm_state': vm_states.BUILDING, - 'task_state': task_states.SCHEDULING, - 'image_ref': None, - 'access_ip_v4': '1.2.3.4', - 'access_ip_v6': '::1', - 'info_cache': jsonutils.dumps(info_cache.obj_to_primitive()), - 'security_groups': jsonutils.dumps( - objects.SecurityGroupList().obj_to_primitive()), - 'config_drive': False, - 'key_name': None, - 'locked_by': None, - 'request_spec': _req_spec_to_db_format(req_spec), 'instance': jsonutils.dumps(instance.obj_to_primitive()), 'created_at': datetime.datetime(2016, 1, 16), 'updated_at': datetime.datetime(2016, 1, 16), @@ -97,19 +62,7 @@ def fake_req_obj(ctxt, db_req=None): continue if isinstance(req_obj.fields[field], fields.ObjectField): value = value - if field == 'request_spec': - req_spec = objects.RequestSpec._from_db_object(context, - objects.RequestSpec(), value) - req_obj.request_spec = req_spec - elif field == 'info_cache': - setattr(req_obj, field, - objects.InstanceInfoCache.obj_from_primitive( - jsonutils.loads(value))) - elif field == 'security_groups': - setattr(req_obj, field, - objects.SecurityGroupList.obj_from_primitive( - jsonutils.loads(value))) - elif field == 'instance': + if field == 'instance': req_obj.instance = objects.Instance.obj_from_primitive( jsonutils.loads(value)) elif field == 'instance_metadata': diff --git a/nova/tests/unit/objects/test_build_request.py b/nova/tests/unit/objects/test_build_request.py index 2531375057..41462924ca 100644 --- a/nova/tests/unit/objects/test_build_request.py +++ b/nova/tests/unit/objects/test_build_request.py @@ -33,11 +33,8 @@ class _TestBuildRequestObject(object): req_obj = build_request.BuildRequest.get_by_instance_uuid(self.context, fake_req['instance_uuid']) - self.assertEqual(fake_req['request_spec']['instance_uuid'], - req_obj.request_spec.instance_uuid) self.assertEqual(fake_req['instance_uuid'], req_obj.instance_uuid) self.assertEqual(fake_req['project_id'], req_obj.project_id) - self.assertIsInstance(req_obj.request_spec, objects.RequestSpec) self.assertIsInstance(req_obj.instance, objects.Instance) get_by_uuid.assert_called_once_with(self.context, fake_req['instance_uuid']) @@ -67,19 +64,13 @@ class _TestBuildRequestObject(object): build_request.BuildRequest.get_by_instance_uuid, self.context, fake_req['instance_uuid']) - @mock.patch.object(build_request.BuildRequest, - '_create_in_db') - def test_create(self, create_in_db): + def test_create(self): fake_req = fake_build_request.fake_db_req() req_obj = fake_build_request.fake_req_obj(self.context, fake_req) def _test_create_args(self2, context, changes): - for field in [fields for fields in - build_request.BuildRequest.fields if fields not in - ['created_at', 'updated_at', 'request_spec', 'id']]: + for field in ['instance_uuid', 'project_id']: self.assertEqual(fake_req[field], changes[field]) - self.assertEqual(fake_req['request_spec']['id'], - changes['request_spec_id']) self.assertEqual( jsonutils.dumps(req_obj.instance.obj_to_primitive()), changes['instance']) diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index 9f3b2e592b..0dde785bd6 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1106,7 +1106,7 @@ object_data = { 'BandwidthUsageList': '1.2-5fe7475ada6fe62413cbfcc06ec70746', 'BlockDeviceMapping': '1.17-5e094927f1251770dcada6ab05adfcdb', 'BlockDeviceMappingList': '1.17-1e568eecb91d06d4112db9fd656de235', - 'BuildRequest': '1.0-fea0b079bddc45f3150f16be5515a2a8', + 'BuildRequest': '1.0-c6cd434db5cbdb4d1ebb935424261377', 'CellMapping': '1.0-7f1a7e85a22bbb7559fc730ab658b9bd', 'ComputeNode': '1.16-2436e5b836fa0306a3c4e6d9e5ddacec', 'ComputeNodeList': '1.14-3b6f4f5ade621c40e70cb116db237844',