Finished cleaning up the openstack servers API, it no longer touches the database directly. Also cleaned up similar things in ec2 API and refactored a couple methods in nova.compute.api to accomodate this work.

9 files changed, 94 insertions, 119 deletions

diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index 7978e08a0..4eef5e1ef 100644
--- a/nova/api/ec2/cloud.py
+++ b/nova/api/ec2/cloud.py
@@ -41,7 +41,6 @@ from nova import rpc
 from nova import utils
 from nova.compute import api as compute_api
 from nova.compute import instance_types
-from nova.image.s3 import S3ImageService
 
 
 FLAGS = flags.FLAGS
@@ -94,8 +93,9 @@ class CloudController(object):
 """
     def __init__(self):
         self.network_manager = utils.import_object(FLAGS.network_manager)
-        self.compute_api = compute_api.ComputeAPI(self.network_manager)
-        self.image_service = S3ImageService()
+        self.image_service = utils.import_object(FLAGS.image_service)
+        self.compute_api = compute_api.ComputeAPI(self.network_manager,
+                                                  self.image_service)
         self.setup()
 
     def __str__(self):
@@ -119,7 +119,7 @@ class CloudController(object):
 
     def _get_mpi_data(self, context, project_id):
         result = {}
-        for instance in db.instance_get_all_by_project(context, project_id):
+        for instance in self.compute_api.get_instances(context, project_id):
             if instance['fixed_ip']:
                 line = '%s slots=%d' % (instance['fixed_ip']['address'],
                                         instance['vcpus'])
@@ -438,7 +438,7 @@ class CloudController(object):
         # instance_id is passed in as a list of instances
         ec2_id = instance_id[0]
         internal_id = ec2_id_to_internal_id(ec2_id)
-        instance_ref = db.instance_get_by_internal_id(context, internal_id)
+        instance_ref = self.compute_api.get_instance(context, internal_id)
         output = rpc.call(context,
                           '%s.%s' % (FLAGS.compute_topic,
                                      instance_ref['host']),
@@ -535,7 +535,7 @@ class CloudController(object):
         if volume_ref['attach_status'] == "attached":
             raise exception.ApiError("Volume is already attached")
         internal_id = ec2_id_to_internal_id(instance_id)
-        instance_ref = db.instance_get_by_internal_id(context, internal_id)
+        instance_ref = self.compute_api.get_instance(context, internal_id)
         host = instance_ref['host']
         rpc.cast(context,
                  db.queue_get_for(context, FLAGS.compute_topic, host),
@@ -613,11 +613,7 @@ class CloudController(object):
             instances = db.instance_get_all_by_reservation(context,
                                                            reservation_id)
         else:
-            if context.user.is_admin():
-                instances = db.instance_get_all(context)
-            else:
-                instances = db.instance_get_all_by_project(context,
-                                                           context.project_id)
+            instances = self.compute_api.get_instances(context)
         for instance in instances:
             if not context.user.is_admin():
                 if instance['image_id'] == FLAGS.vpn_image_id:
@@ -714,7 +710,7 @@ class CloudController(object):
 
     def associate_address(self, context, instance_id, public_ip, **kwargs):
         internal_id = ec2_id_to_internal_id(instance_id)
-        instance_ref = db.instance_get_by_internal_id(context, internal_id)
+        instance_ref = self.compute_api.get_instance(context, internal_id)
         fixed_address = db.instance_get_fixed_address(context,
                                                       instance_ref['id'])
         floating_ip_ref = db.floating_ip_get_by_address(context, public_ip)
@@ -750,13 +746,12 @@ class CloudController(object):
         max_count = int(kwargs.get('max_count', 1))
         instances = self.compute_api.create_instances(context,
             instance_types.get_by_type(kwargs.get('instance_type', None)),
-            self.image_service,
             kwargs['image_id'],
             min_count=int(kwargs.get('min_count', max_count)),
             max_count=max_count,
             kernel_id=kwargs.get('kernel_id'),
             ramdisk_id=kwargs.get('ramdisk_id'),
-            name=kwargs.get('display_name'),
+            display_name=kwargs.get('display_name'),
             description=kwargs.get('display_description'),
             user_data=kwargs.get('user_data', ''),
             key_name=kwargs.get('key_name'),
@@ -801,7 +796,7 @@ class CloudController(object):
                 changes[field] = kwargs[field]
         if changes:
             internal_id = ec2_id_to_internal_id(ec2_id)
-            inst = db.instance_get_by_internal_id(context, internal_id)
+            inst = self.compute_api.get_instance(context, internal_id)
             db.instance_update(context, inst['id'], kwargs)
         return True
 
diff --git a/nova/api/openstack/servers.py b/nova/api/openstack/servers.py
index a9da14867..b644876b0 100644
--- a/nova/api/openstack/servers.py
+++ b/nova/api/openstack/servers.py
@@ -15,23 +15,17 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import webob
 from webob import exc
 
 from nova import context
 from nova import exception
-from nova import flags
-from nova import rpc
-from nova import utils
 from nova import wsgi
 from nova.api.openstack import faults
+from nova.auth import manager as auth_manager
 from nova.compute import api as compute_api
 from nova.compute import instance_types
 from nova.compute import power_state
 import nova.api.openstack
-import nova.image.service
-
-FLAGS = flags.FLAGS
 
 
 def _entity_list(entities):
@@ -79,10 +73,7 @@ class Controller(wsgi.Controller):
                 "server": ["id", "imageId", "name", "flavorId", "hostId",
                            "status", "progress"]}}}
 
-    def __init__(self, db_driver=None):
-        if not db_driver:
-            db_driver = FLAGS.db_driver
-        self.db_driver = utils.import_object(db_driver)
+    def __init__(self):
         self.compute_api = compute_api.ComputeAPI()
         super(Controller, self).__init__()
 
@@ -101,7 +92,7 @@ class Controller(wsgi.Controller):
         """
         user_id = req.environ['nova.context']['user']['id']
         ctxt = context.RequestContext(user_id, user_id)
-        instance_list = self.db_driver.instance_get_all_by_user(ctxt, user_id)
+        instance_list = self.compute_api.get_instances(ctxt)
         limited_list = nova.api.openstack.limited(instance_list, req)
         res = [entity_maker(inst)['server'] for inst in limited_list]
         return _entity_list(res)
@@ -110,7 +101,7 @@ class Controller(wsgi.Controller):
         """ Returns server details by server id """
         user_id = req.environ['nova.context']['user']['id']
         ctxt = context.RequestContext(user_id, user_id)
-        inst = self.db_driver.instance_get_by_internal_id(ctxt, int(id))
+        inst = self.compute_api.get_instance(ctxt, int(id))
         if inst:
             if inst.user_id == user_id:
                 return _entity_detail(inst)
@@ -134,12 +125,11 @@ class Controller(wsgi.Controller):
 
         user_id = req.environ['nova.context']['user']['id']
         ctxt = context.RequestContext(user_id, user_id)
-        key_pair = self.db_driver.key_pair_get_all_by_user(None, user_id)[0]
+        key_pair = auth_manager.AuthManager.get_key_pairs(ctxt)[0]
         instances = self.compute_api.create_instances(ctxt,
             instance_types.get_by_flavor_id(env['server']['flavorId']),
-            utils.import_object(FLAGS.image_service),
             env['server']['imageId'],
-            name=env['server']['name'],
+            display_name=env['server']['name'],
             description=env['server']['name'],
             key_name=key_pair['name'],
             key_data=key_pair['public_key'])
@@ -149,27 +139,24 @@ class Controller(wsgi.Controller):
         """ Updates the server name or password """
         user_id = req.environ['nova.context']['user']['id']
         ctxt = context.RequestContext(user_id, user_id)
-
         inst_dict = self._deserialize(req.body, req)
-
         if not inst_dict:
             return faults.Fault(exc.HTTPUnprocessableEntity())
 
-        instance = self.db_driver.instance_get_by_internal_id(ctxt, int(id))
-        if not instance or instance.user_id != user_id:
-            return faults.Fault(exc.HTTPNotFound())
-
         update_dict = {}
         if 'adminPass' in inst_dict['server']:
             update_dict['admin_pass'] = inst_dict['server']['adminPass']
         if 'name' in inst_dict['server']:
             update_dict['display_name'] = inst_dict['server']['name']
 
-        self.compute_api.update_instance(ctxt, instance['id'], update_dict)
+        try:
+            self.compute_api.update_instance(ctxt, instance['id'], update_dict)
+        except exception.NotFound:
+            return faults.Fault(exc.HTTPNotFound())
         return exc.HTTPNoContent()
 
     def action(self, req, id):
-        """ multi-purpose method used to reboot, rebuild, and
+        """ Multi-purpose method used to reboot, rebuild, and
         resize a server """
         user_id = req.environ['nova.context']['user']['id']
         ctxt = context.RequestContext(user_id, user_id)
@@ -177,10 +164,11 @@ class Controller(wsgi.Controller):
         try:
             reboot_type = input_dict['reboot']['type']
         except Exception:
-            raise faults.Fault(webob.exc.HTTPNotImplemented())
-        inst_ref = self.db.instance_get_by_internal_id(ctxt, int(id))
-        if not inst_ref or (inst_ref and not inst_ref.user_id == user_id):
+            raise faults.Fault(exc.HTTPNotImplemented())
+        try:
+            # TODO(gundlach): pass reboot_type, support soft reboot in
+            # virt driver
+            self.compute_api.reboot(ctxt, id)
+        except:
             return faults.Fault(exc.HTTPUnprocessableEntity())
-        # TODO(gundlach): pass reboot_type, support soft reboot in
-        # virt driver
-        self.compute_api.reboot(ctxt, id)
+        return exc.HTTPNoContent()
diff --git a/nova/auth/manager.py b/nova/auth/manager.py
index 7b2b68161..11c3bd6df 100644
--- a/nova/auth/manager.py
+++ b/nova/auth/manager.py
@@ -624,6 +624,10 @@ class AuthManager(object):
         with self.driver() as drv:
             drv.modify_user(uid, access_key, secret_key, admin)
 
+    @staticmethod
+    def get_key_pairs(context):
+        return db.key_pair_get_all_by_user(context.elevated(), context.user_id)
+
     def get_credentials(self, user, project=None):
         """Get credential zip for user in project"""
         if not isinstance(user, User):
diff --git a/nova/compute/api.py b/nova/compute/api.py
index 457d6e27f..995bed91b 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -44,16 +44,19 @@ def generate_default_hostname(internal_id):
 class ComputeAPI(base.Base):
     """API for interacting with the compute manager."""
 
-    def __init__(self, network_manager=None, **kwargs):
+    def __init__(self, network_manager=None, image_service=None, **kwargs):
         if not network_manager:
             network_manager = utils.import_object(FLAGS.network_manager)
         self.network_manager = network_manager
+        if not image_service:
+            image_service = utils.import_object(FLAGS.image_service)
+        self.image_service = image_service
         super(ComputeAPI, self).__init__(**kwargs)
 
-    def create_instances(self, context, instance_type, image_service, image_id,
-                         min_count=1, max_count=1, kernel_id=None,
-                         ramdisk_id=None, name='', description='',
-                         user_data='', key_name=None, key_data=None,
+    def create_instances(self, context, instance_type, image_id, min_count=1,
+                         max_count=1, kernel_id=None, ramdisk_id=None,
+                         display_name='', description='', user_data='',
+                         key_name=None, key_data=None,
                          security_group='default',
                          generate_hostname=generate_default_hostname):
         """Create the number of instances requested if quote and
@@ -70,15 +73,15 @@ class ComputeAPI(base.Base):
 
         is_vpn = image_id == FLAGS.vpn_image_id
         if not is_vpn:
-            image = image_service.show(context, image_id)
+            image = self.image_service.show(context, image_id)
             if kernel_id is None:
                 kernel_id = image.get('kernelId', FLAGS.default_kernel)
             if ramdisk_id is None:
                 ramdisk_id = image.get('ramdiskId', FLAGS.default_ramdisk)
 
             # Make sure we have access to kernel and ramdisk
-            image_service.show(context, kernel_id)
-            image_service.show(context, ramdisk_id)
+            self.image_service.show(context, kernel_id)
+            self.image_service.show(context, ramdisk_id)
 
         if security_group is None:
             security_group = ['default']
@@ -111,7 +114,7 @@ class ComputeAPI(base.Base):
             'memory_mb': type_data['memory_mb'],
             'vcpus': type_data['vcpus'],
             'local_gb': type_data['local_gb'],
-            'display_name': name,
+            'display_name': display_name,
             'display_description': description,
             'key_name': key_name,
             'key_data': key_data}
@@ -123,14 +126,25 @@ class ComputeAPI(base.Base):
             instance = dict(mac_address=utils.generate_mac(),
                             launch_index=num,
                             **base_options)
-            instance_ref = self.create_instance(context, security_groups,
-                                                **instance)
-            instance_id = instance_ref['id']
-            internal_id = instance_ref['internal_id']
-            hostname = generate_hostname(internal_id)
-            self.update_instance(context, instance_id, hostname=hostname)
-            instances.append(dict(id=instance_id, internal_id=internal_id,
-                             hostname=hostname, **instance))
+            instance = self.db.instance_create(context, instance)
+            instance_id = instance['id']
+            internal_id = instance['internal_id']
+
+            elevated = context.elevated()
+            if not security_groups:
+                security_groups = []
+            for security_group_id in security_groups:
+                self.db.instance_add_security_group(elevated,
+                                                    instance_id,
+                                                    security_group_id)
+
+            # Set sane defaults if not specified
+            updates = dict(hostname=generate_hostname(internal_id))
+            if 'display_name' not in instance:
+                updates['display_name'] = "Server %s" % internal_id
+
+            instance = self.update_instance(context, instance_id, **updates)
+            instances.append(instance)
 
             # TODO(vish): This probably should be done in the scheduler
             #             or in compute as a call.  The network should be
@@ -165,39 +179,6 @@ class ComputeAPI(base.Base):
                       'project_id': context.project_id}
             group = db.security_group_create(context, values)
 
-    def create_instance(self, context, security_groups=None, **kwargs):
-        """Creates the instance in the datastore and returns the
-        new instance as a mapping
-
-        :param context: The security context
-        :param security_groups: list of security group ids to
-                                attach to the instance
-        :param kwargs: All additional keyword args are treated
-                       as data fields of the instance to be
-                       created
-
-        :retval Returns a mapping of the instance information
-                that has just been created
-
-        """
-        instance_ref = self.db.instance_create(context, kwargs)
-        inst_id = instance_ref['id']
-        # Set sane defaults if not specified
-        if kwargs.get('display_name') is None:
-            display_name = "Server %s" % instance_ref['internal_id']
-            instance_ref['display_name'] = display_name
-            self.db.instance_update(context, inst_id,
-                                    {'display_name': display_name})
-
-        elevated = context.elevated()
-        if not security_groups:
-            security_groups = []
-        for security_group_id in security_groups:
-            self.db.instance_add_security_group(elevated,
-                                                inst_id,
-                                                security_group_id)
-        return instance_ref
-
     def update_instance(self, context, instance_id, **kwargs):
         """Updates the instance in the datastore.
 
@@ -210,7 +191,7 @@ class ComputeAPI(base.Base):
         :retval None
 
         """
-        self.db.instance_update(context, instance_id, kwargs)
+        return self.db.instance_update(context, instance_id, kwargs)
 
     def delete_instance(self, context, instance_id):
         logging.debug("Going to try and terminate %d" % instance_id)
@@ -264,6 +245,19 @@ class ComputeAPI(base.Base):
         else:
             self.db.instance_destroy(context, instance['id'])
 
+    def get_instances(self, context, project_id=None):
+        if project_id or not context.is_admin:
+            if not context.project:
+                return self.db.instance_get_all_by_user(context,
+                                                        context.user_id)
+            if project_id is None:
+                project_id = context.project_id
+            return self.db.instance_get_all_by_project(context, project_id)
+        return self.db.instance_get_all(context)
+
+    def get_instance(self, context, instance_id):
+        return self.db.instance_get_by_internal_id(context, instance_id)
+
     def reboot(self, context, instance_id):
         """Reboot the given instance."""
         instance = self.db.instance_get_by_internal_id(context, instance_id)
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index dd9649054..ef58f3490 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -732,6 +732,7 @@ def instance_update(context, instance_id, values):
         instance_ref = instance_get(context, instance_id, session=session)
         instance_ref.update(values)
         instance_ref.save(session=session)
+        return instance_ref
 
 
 def instance_add_security_group(context, instance_id, security_group_id):
diff --git a/nova/flags.py b/nova/flags.py
index 1f94feb08..c6578023d 100644
--- a/nova/flags.py
+++ b/nova/flags.py
@@ -259,7 +259,7 @@ DEFINE_string('scheduler_manager', 'nova.scheduler.manager.SchedulerManager',
               'Manager for scheduler')
 
 # The service to use for image search and retrieval
-DEFINE_string('image_service', 'nova.image.local.LocalImageService',
+DEFINE_string('image_service', 'nova.image.s3.S3ImageService',
               'The service to use for retrieving and searching for images.')
 
 DEFINE_string('host', socket.gethostname(),
diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py
index 7c0343942..c3f129a32 100644
--- a/nova/tests/api/openstack/fakes.py
+++ b/nova/tests/api/openstack/fakes.py
@@ -67,8 +67,7 @@ def fake_wsgi(self, req):
 def stub_out_key_pair_funcs(stubs):
     def key_pair(context, user_id):
         return [dict(name='key', public_key='public_key')]
-    stubs.Set(nova.db.api, 'key_pair_get_all_by_user',
-        key_pair)
+    stubs.Set(nova.db, 'key_pair_get_all_by_user', key_pair)
 
 
 def stub_out_image_service(stubs):
diff --git a/nova/tests/api/openstack/test_servers.py b/nova/tests/api/openstack/test_servers.py
index 46b9c5348..8444b6fce 100644
--- a/nova/tests/api/openstack/test_servers.py
+++ b/nova/tests/api/openstack/test_servers.py
@@ -48,7 +48,7 @@ def return_security_group(context, instance_id, security_group_id):
 
 
 def instance_update(context, instance_id, kwargs):
-    pass
+    return stub_instance(instance_id)
 
 
 def instance_address(context, instance_id):
@@ -106,11 +106,11 @@ class ServersTest(unittest.TestCase):
             i += 1
 
     def test_create_instance(self):
-        def server_update(context, id, params):
-            pass
-
         def instance_create(context, inst):
-            return {'id': 1, 'internal_id': 1}
+            return {'id': 1, 'internal_id': 1, 'display_name': ''}
+
+        def server_update(context, id, params):
+            return instance_create(context, id)
 
         def fake_method(*args, **kwargs):
             pass
diff --git a/nova/tests/compute_unittest.py b/nova/tests/compute_unittest.py
index a55449739..6f3ef96cb 100644
--- a/nova/tests/compute_unittest.py
+++ b/nova/tests/compute_unittest.py
@@ -72,33 +72,27 @@ class ComputeTestCase(test.TrialTestCase):
         """Verify that an instance cannot be created without a display_name."""
         cases = [dict(), dict(display_name=None)]
         for instance in cases:
-            ref = self.compute_api.create_instance(self.context, None,
-                                                   **instance)
+            ref = self.compute_api.create_instances(self.context,
+                FLAGS.default_instance_type, None, **instance)
             try:
-                self.assertNotEqual(ref.display_name, None)
+                self.assertNotEqual(ref[0].display_name, None)
             finally:
-                db.instance_destroy(self.context, ref['id'])
+                db.instance_destroy(self.context, ref[0]['id'])
 
     def test_create_instance_associates_security_groups(self):
-        """Make sure create_instance associates security groups"""
-        inst = {}
-        inst['user_id'] = self.user.id
-        inst['project_id'] = self.project.id
+        """Make sure create_instances associates security groups"""
         values = {'name': 'default',
                   'description': 'default',
                   'user_id': self.user.id,
                   'project_id': self.project.id}
         group = db.security_group_create(self.context, values)
-        ref = self.compute_api.create_instance(self.context,
-                                               security_groups=[group['id']],
-                                               **inst)
-        # reload to get groups
-        instance_ref = db.instance_get(self.context, ref['id'])
+        ref = self.compute_api.create_instances(self.context,
+            FLAGS.default_instance_type, None, security_group=['default'])
         try:
-            self.assertEqual(len(instance_ref['security_groups']), 1)
+            self.assertEqual(len(ref[0]['security_groups']), 1)
         finally:
             db.security_group_destroy(self.context, group['id'])
-            db.instance_destroy(self.context, instance_ref['id'])
+            db.instance_destroy(self.context, ref[0]['id'])
 
     @defer.inlineCallbacks
     def test_run_terminate(self):