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 <uuid>' vs
'Server <id>'.  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

4 files changed, 185 insertions, 99 deletions

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: