From 936140de2c42b8e1b4cf1edde1e6fb25bcd75c59 Mon Sep 17 00:00:00 2001 From: Chris Behrens Date: Mon, 25 Jun 2012 21:14:11 +0000 Subject: Re-factor instance DB creation This patch speeds up the DB creation process considerably in deployments with a considerable amount of instances. It also cleans up a lot of the code which ends up creating the instance DB record. The biggest improvement in this patch is removing unnecessary joins in security_groups DB calls. This also reduces the number of DB calls needed to create an instance DB record in general. Side effect of this patch is the default 'display_name' for an instance when it (or hostname) is not specified is now 'Server ' vs 'Server '. Because of the use of 'id', it required creating the DB record, then updating the record later after we new the 'id'. This is gone. Fixes bug 1017722 Change-Id: I9b7d48644a7abe075545c2c11399351b6a37939c --- nova/compute/api.py | 163 ++++++++++++++++++------------------- nova/db/api.py | 5 ++ nova/db/sqlalchemy/api.py | 101 +++++++++++++++++++---- nova/tests/compute/test_compute.py | 15 +++- 4 files changed, 185 insertions(+), 99 deletions(-) (limited to 'nova') diff --git a/nova/compute/api.py b/nova/compute/api.py index caec8f4b6..05f4caa29 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -398,8 +398,6 @@ class API(base.Base): kernel_id, ramdisk_id = self._handle_kernel_and_ramdisk( context, kernel_id, ramdisk_id, image, image_service) - self.security_group_api.ensure_default(context) - if key_data is None and key_name: key_pair = self.db.key_pair_get(context, context.user_id, key_name) key_data = key_pair['public_key'] @@ -521,9 +519,6 @@ class API(base.Base): """tell vm driver to create ephemeral/swap device at boot time by updating BlockDeviceMapping """ - instance_type = (instance_type or - instance_types.get_default_instance_type()) - for bdm in block_device.mappings_prepend_dev(mappings): LOG.debug(_("bdm %s"), bdm) @@ -583,6 +578,74 @@ class API(base.Base): self.db.block_device_mapping_update_or_create(elevated_context, values) + def _populate_instance_for_bdm(self, context, instance, instance_type, + image, block_device_mapping): + """Populate instance block device mapping information.""" + # FIXME(comstud): Why do the block_device_mapping DB calls + # require elevated context? + elevated = context.elevated() + instance_uuid = instance['uuid'] + mappings = image['properties'].get('mappings', []) + if mappings: + instance['shutdown_terminate'] = False + self._update_image_block_device_mapping(elevated, + instance_type, instance_uuid, mappings) + + image_bdm = image['properties'].get('block_device_mapping', []) + for mapping in (image_bdm, block_device_mapping): + if not mapping: + continue + instance['shutdown_terminate'] = False + self._update_block_device_mapping(elevated, + instance_type, instance_uuid, mapping) + + def _populate_instance_names(self, instance): + """Populate instance display_name and hostname.""" + display_name = instance.get('display_name') + hostname = instance.get('hostname') + + if display_name is None: + display_name = self._default_display_name(instance['uuid']) + instance['display_name'] = display_name + if hostname is None: + hostname = display_name + instance['hostname'] = utils.sanitize_hostname(hostname) + + def _default_display_name(self, instance_uuid): + return "Server %s" % instance_uuid + + def _populate_instance_for_create(self, base_options, image, + security_groups): + """Build the beginning of a new instance.""" + + instance = base_options + if not instance.get('uuid'): + # Generate the instance_uuid here so we can use it + # for additional setup before creating the DB entry. + instance['uuid'] = str(utils.gen_uuid()) + + instance['launch_index'] = 0 + instance['vm_state'] = vm_states.BUILDING + instance['task_state'] = task_states.SCHEDULING + instance['architecture'] = image['properties'].get('architecture') + instance['info_cache'] = {'network_info': '[]'} + + # Store image properties so we can use them later + # (for notifications, etc). Only store what we can. + instance.setdefault('system_metadata', {}) + for key, value in image['properties'].iteritems(): + new_value = str(value)[:255] + instance['system_metadata']['image_%s' % key] = new_value + + # Use 'default' security_group if none specified. + if security_groups is None: + security_groups = ['default'] + elif not isinstance(security_groups, list): + security_groups = [security_groups] + instance['security_groups'] = security_groups + + return instance + #NOTE(bcwaldon): No policy check since this is only used by scheduler and # the compute api. That should probably be cleaned up, though. def create_db_entry_for_new_instance(self, context, instance_type, image, @@ -594,83 +657,26 @@ class API(base.Base): This is called by the scheduler after a location for the instance has been determined. """ - elevated = context.elevated() - if security_group is None: - security_group = ['default'] - if not isinstance(security_group, list): - security_group = [security_group] - - security_groups = [] - for security_group_name in security_group: - group = self.db.security_group_get_by_name(context, - context.project_id, - security_group_name) - security_groups.append(group['id']) - - # Store image properties so we can use them later - # (for notifications, etc). Only store what we can. - base_options.setdefault('system_metadata', {}) - for key, value in image['properties'].iteritems(): - new_value = str(value)[:255] - base_options['system_metadata']['image_%s' % key] = new_value + instance = self._populate_instance_for_create(base_options, + image, security_group) - base_options.setdefault('launch_index', 0) - instance = self.db.instance_create(context, base_options) + self._populate_instance_names(instance) - # Commit the reservations - if reservations: - QUOTAS.commit(context, reservations) + self._populate_instance_for_bdm(context, instance, + instance_type, image, block_device_mapping) - instance_id = instance['id'] - instance_uuid = instance['uuid'] + instance = self.db.instance_create(context, instance) # send a state update notification for the initial create to # show it going from non-existent to BUILDING notifications.send_update_with_states(context, instance, None, vm_states.BUILDING, None, None, service="api") - for security_group_id in security_groups: - self.db.instance_add_security_group(elevated, - instance_uuid, - security_group_id) - - # BlockDeviceMapping table - self._update_image_block_device_mapping(elevated, instance_type, - instance_uuid, image['properties'].get('mappings', [])) - self._update_block_device_mapping(elevated, instance_type, - instance_uuid, - image['properties'].get('block_device_mapping', [])) - # override via command line option - self._update_block_device_mapping(elevated, instance_type, - instance_uuid, block_device_mapping) - - # Set sane defaults if not specified - updates = {} - - display_name = instance.get('display_name') - if display_name is None: - display_name = self._default_display_name(instance_id) - - hostname = instance.get('hostname') - if hostname is None: - hostname = display_name - - updates['display_name'] = display_name - updates['hostname'] = utils.sanitize_hostname(hostname) - updates['vm_state'] = vm_states.BUILDING - updates['task_state'] = task_states.SCHEDULING - - updates['architecture'] = image['properties'].get('architecture') - - if (image['properties'].get('mappings', []) or - image['properties'].get('block_device_mapping', []) or - block_device_mapping): - updates['shutdown_terminate'] = False - - return self.update(context, instance, **updates) + # Commit the reservations + if reservations: + QUOTAS.commit(context, reservations) - def _default_display_name(self, instance_id): - return "Server %s" % instance_id + return instance def _schedule_run_instance(self, use_call, @@ -749,7 +755,7 @@ class API(base.Base): # only going to create 1 instance. # This speeds up API responses for builds # as we don't need to wait for the scheduler. - create_instance_here = max_count == 1 + create_instance_here = max_count == 1 or max_count == None (instances, reservation_id) = self._create_instance( context, instance_type, @@ -1926,16 +1932,7 @@ class SecurityGroupAPI(base.Base): :param context: the security context """ - try: - self.db.security_group_get_by_name(context, - context.project_id, - 'default') - except exception.NotFound: - values = {'name': 'default', - 'description': 'default', - 'user_id': context.user_id, - 'project_id': context.project_id} - self.db.security_group_create(context, values) + self.db.security_group_ensure_default(context) def create(self, context, name, description): try: diff --git a/nova/db/api.py b/nova/db/api.py index e0d150506..d3c55c332 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -1276,6 +1276,11 @@ def security_group_create(context, values): return IMPL.security_group_create(context, values) +def security_group_ensure_default(context): + """Ensure default security group exists for a project_id.""" + return IMPL.security_group_ensure_default(context) + + def security_group_destroy(context, security_group_id): """Deletes a security group.""" return IMPL.security_group_destroy(context, security_group_id) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index eabd03a22..4e7879e20 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -1329,14 +1329,34 @@ def instance_create(context, values): instance_ref = models.Instance() if not values.get('uuid'): values['uuid'] = str(utils.gen_uuid()) + instance_ref['info_cache'] = models.InstanceInfoCache() + info_cache = values.pop('info_cache', None) + if info_cache is not None: + instance_ref['info_cache'].update(info_cache) + security_groups = values.pop('security_groups', []) instance_ref.update(values) + def _get_sec_group_models(session, security_groups): + models = [] + default_group = security_group_ensure_default(context, + session=session) + if 'default' in security_groups: + models.append(default_group) + # Generate a new list, so we don't modify the original + security_groups = [x for x in security_groups if x != 'default'] + if security_groups: + models.extend(_security_group_get_by_names(context, + session, context.project_id, security_groups)) + return models + session = get_session() with session.begin(): + instance_ref.security_groups = _get_sec_group_models(session, + security_groups) instance_ref.save(session=session) - - # and creat the info_cache table entry for instance - instance_info_cache_create(context, {'instance_id': instance_ref['uuid']}) + # NOTE(comstud): This forces instance_type to be loaded so it + # exists in the ref when we return. Fixes lazy loading issues. + instance_ref.instance_type return instance_ref @@ -3331,10 +3351,33 @@ def block_device_mapping_destroy_by_instance_and_volume(context, instance_uuid, ################### def _security_group_get_query(context, session=None, read_deleted=None, - project_only=False): - return model_query(context, models.SecurityGroup, session=session, - read_deleted=read_deleted, project_only=project_only).\ - options(joinedload_all('rules')) + project_only=False, join_rules=True): + query = model_query(context, models.SecurityGroup, session=session, + read_deleted=read_deleted, project_only=project_only) + if join_rules: + query = query.options(joinedload_all('rules')) + return query + + +def _security_group_get_by_names(context, session, project_id, group_names): + """ + Get security group models for a project by a list of names. + Raise SecurityGroupNotFoundForProject for a name not found. + """ + query = _security_group_get_query(context, session=session, + read_deleted="no", join_rules=False).\ + filter_by(project_id=project_id).\ + filter(models.SecurityGroup.name.in_(group_names)) + sg_models = query.all() + if len(sg_models) == len(group_names): + return sg_models + # Find the first one missing and raise + group_names_from_models = [x.name for x in sg_models] + for group_name in group_names: + if group_name not in group_names_from_models: + raise exception.SecurityGroupNotFoundForProject( + project_id=project_id, security_group_id=group_name) + # Not Reached @require_context @@ -3358,13 +3401,23 @@ def security_group_get(context, security_group_id, session=None): @require_context -def security_group_get_by_name(context, project_id, group_name): - result = _security_group_get_query(context, read_deleted="no").\ - filter_by(project_id=project_id).\ - filter_by(name=group_name).\ - options(joinedload_all('instances')).\ - first() +def security_group_get_by_name(context, project_id, group_name, + columns_to_join=None, session=None): + if session is None: + session = get_session() + query = _security_group_get_query(context, session=session, + read_deleted="no", join_rules=False).\ + filter_by(project_id=project_id).\ + filter_by(name=group_name) + + if columns_to_join is None: + columns_to_join = ['instances', 'rules'] + + for column in columns_to_join: + query = query.options(joinedload_all(column)) + + result = query.first() if not result: raise exception.SecurityGroupNotFoundForProject( project_id=project_id, security_group_id=group_name) @@ -3418,16 +3471,34 @@ def security_group_in_use(context, group_id): @require_context -def security_group_create(context, values): +def security_group_create(context, values, session=None): security_group_ref = models.SecurityGroup() # FIXME(devcamcar): Unless I do this, rules fails with lazy load exception # once save() is called. This will get cleaned up in next orm pass. security_group_ref.rules security_group_ref.update(values) - security_group_ref.save() + if session is None: + session = get_session() + security_group_ref.save(session=session) return security_group_ref +def security_group_ensure_default(context, session=None): + """Ensure default security group exists for a project_id.""" + try: + default_group = security_group_get_by_name(context, + context.project_id, 'default', + columns_to_join=[], session=session) + except exception.NotFound: + values = {'name': 'default', + 'description': 'default', + 'user_id': context.user_id, + 'project_id': context.project_id} + default_group = security_group_create(context, values, + session=session) + return default_group + + @require_context def security_group_destroy(context, security_group_id): session = get_session() diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index c816acc95..e77b0fcdb 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -2159,7 +2159,20 @@ class ComputeAPITestCase(BaseTestCase): len(db.instance_get_all(context.get_admin_context()))) def test_default_hostname_generator(self): - cases = [(None, 'server-1'), ('Hello, Server!', 'hello-server'), + fake_uuids = [str(utils.gen_uuid()) for x in xrange(4)] + + orig_populate = self.compute_api._populate_instance_for_create + + def _fake_populate(base_options, *args, **kwargs): + base_options['uuid'] = fake_uuids.pop(0) + return orig_populate(base_options, *args, **kwargs) + + self.stubs.Set(self.compute_api, + '_populate_instance_for_create', + _fake_populate) + + cases = [(None, 'server-%s' % fake_uuids[0]), + ('Hello, Server!', 'hello-server'), ('<}\x1fh\x10e\x08l\x02l\x05o\x12!{>', 'hello'), ('hello_server', 'hello-server')] for display_name, hostname in cases: -- cgit