Merge "Sort results from describe_instances in EC2 API."

6 files changed, 107 insertions, 23 deletions

diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index aa1f2203c..81f0c9a7e 100644
--- a/nova/api/ec2/cloud.py
+++ b/nova/api/ec2/cloud.py
@@ -1160,7 +1160,8 @@ class CloudController(object):
                 # always filter out deleted instances
                 search_opts['deleted'] = False
                 instances = self.compute_api.get_all(context,
-                                                     search_opts=search_opts)
+                                                     search_opts=search_opts,
+                                                     sort_dir='asc')
             except exception.NotFound:
                 instances = []
         for instance in instances:
diff --git a/nova/compute/api.py b/nova/compute/api.py
index 71d7627d5..79e490395 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1028,7 +1028,8 @@ class API(base.Base):
         inst['name'] = instance['name']
         return inst
 
-    def get_all(self, context, search_opts=None):
+    def get_all(self, context, search_opts=None, sort_key='created_at',
+                sort_dir='desc'):
         """Get all instances filtered by one of the given parameters.
 
         If there is no filter and the context is an admin, it will retrieve
@@ -1036,6 +1037,10 @@ class API(base.Base):
 
         Deleted instances will be returned by default, unless there is a
         search option that says otherwise.
+
+        The results will be returned sorted in the order specified by the
+        'sort_dir' parameter using the key specified in the 'sort_key'
+        parameter.
         """
 
         #TODO(bcwaldon): determine the best argument for target here
@@ -1101,7 +1106,8 @@ class API(base.Base):
                     except ValueError:
                         return []
 
-        inst_models = self._get_instances_by_filters(context, filters)
+        inst_models = self._get_instances_by_filters(context, filters,
+                                                     sort_key, sort_dir)
 
         # Convert the models to dictionaries
         instances = []
@@ -1113,7 +1119,7 @@ class API(base.Base):
 
         return instances
 
-    def _get_instances_by_filters(self, context, filters):
+    def _get_instances_by_filters(self, context, filters, sort_key, sort_dir):
         if 'ip6' in filters or 'ip' in filters:
             res = self.network_api.get_instance_uuids_by_ip_filter(context,
                                                                    filters)
@@ -1122,7 +1128,8 @@ class API(base.Base):
             uuids = set([r['instance_uuid'] for r in res])
             filters['uuid'] = uuids
 
-        return self.db.instance_get_all_by_filters(context, filters)
+        return self.db.instance_get_all_by_filters(context, filters, sort_key,
+                                                   sort_dir)
 
     @wrap_check_policy
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.SHUTOFF])
diff --git a/nova/db/api.py b/nova/db/api.py
index 692ff4996..e569a52ab 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -562,9 +562,11 @@ def instance_get_all(context):
     return IMPL.instance_get_all(context)
 
 
-def instance_get_all_by_filters(context, filters):
+def instance_get_all_by_filters(context, filters, sort_key='created_at',
+                                sort_dir='desc'):
     """Get all instances that match all filters."""
-    return IMPL.instance_get_all_by_filters(context, filters)
+    return IMPL.instance_get_all_by_filters(context, filters, sort_key,
+                                            sort_dir)
 
 
 def instance_get_active_by_window(context, begin, end=None, project_id=None):
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 7af4323a9..2136dbd6d 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -40,6 +40,7 @@ from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm import joinedload
 from sqlalchemy.orm import joinedload_all
 from sqlalchemy.sql import func
+from sqlalchemy.sql.expression import asc
 from sqlalchemy.sql.expression import desc
 from sqlalchemy.sql.expression import literal_column
 
@@ -1379,7 +1380,7 @@ def instance_get_all(context):
 
 
 @require_context
-def instance_get_all_by_filters(context, filters):
+def instance_get_all_by_filters(context, filters, sort_key, sort_dir):
     """Return instances that match all filters.  Deleted instances
     will be returned by default, unless there's a filter that says
     otherwise"""
@@ -1406,13 +1407,15 @@ def instance_get_all_by_filters(context, filters):
             return True
         return False
 
+    sort_fn = {'desc': desc, 'asc': asc}
+
     session = get_session()
     query_prefix = session.query(models.Instance).\
             options(joinedload('info_cache')).\
             options(joinedload('security_groups')).\
             options(joinedload('metadata')).\
             options(joinedload('instance_type')).\
-            order_by(desc(models.Instance.created_at))
+            order_by(sort_fn[sort_dir](getattr(models.Instance, sort_key)))
 
     # Make a copy of the filters dictionary to use going forward, as we'll
     # be modifying it and we shouldn't affect the caller's use of it.
diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py
index 433f0bc33..7f56aa479 100644
--- a/nova/tests/api/ec2/test_cloud.py
+++ b/nova/tests/api/ec2/test_cloud.py
@@ -19,6 +19,7 @@
 
 import base64
 import copy
+import datetime
 import functools
 import os
 import string
@@ -712,6 +713,62 @@ class CloudTestCase(test.TestCase):
         db.service_destroy(self.context, comp1['id'])
         db.service_destroy(self.context, comp2['id'])
 
+    def test_describe_instances_sorting(self):
+        """Makes sure describe_instances works and is sorted as expected."""
+        self.flags(use_ipv6=True)
+
+        self._stub_instance_get_with_fixed_ips('get_all')
+        self._stub_instance_get_with_fixed_ips('get')
+
+        image_uuid = 'cedef40a-ed67-4d10-800e-17455edce175'
+        inst_base = {
+                'reservation_id': 'a',
+                'image_ref': image_uuid,
+                'instance_type_id': 1,
+                'vm_state': 'active'
+        }
+
+        inst1_kwargs = {}
+        inst1_kwargs.update(inst_base)
+        inst1_kwargs['host'] = 'host1'
+        inst1_kwargs['hostname'] = 'server-1111'
+        inst1_kwargs['created_at'] = datetime.datetime(2012, 5, 1, 1, 1, 1)
+        inst1 = db.instance_create(self.context, inst1_kwargs)
+
+        inst2_kwargs = {}
+        inst2_kwargs.update(inst_base)
+        inst2_kwargs['host'] = 'host2'
+        inst2_kwargs['hostname'] = 'server-2222'
+        inst2_kwargs['created_at'] = datetime.datetime(2012, 2, 1, 1, 1, 1)
+        inst2 = db.instance_create(self.context, inst2_kwargs)
+
+        inst3_kwargs = {}
+        inst3_kwargs.update(inst_base)
+        inst3_kwargs['host'] = 'host3'
+        inst3_kwargs['hostname'] = 'server-3333'
+        inst3_kwargs['created_at'] = datetime.datetime(2012, 2, 5, 1, 1, 1)
+        inst3 = db.instance_create(self.context, inst3_kwargs)
+
+        comp1 = db.service_create(self.context, {'host': 'host1',
+                                                 'availability_zone': 'zone1',
+                                                 'topic': "compute"})
+
+        comp2 = db.service_create(self.context, {'host': 'host2',
+                                                 'availability_zone': 'zone2',
+                                                 'topic': "compute"})
+
+        result = self.cloud.describe_instances(self.context)
+        result = result['reservationSet'][0]['instancesSet']
+        self.assertEqual(result[0]['launchTime'], inst2_kwargs['created_at'])
+        self.assertEqual(result[1]['launchTime'], inst3_kwargs['created_at'])
+        self.assertEqual(result[2]['launchTime'], inst1_kwargs['created_at'])
+
+        db.instance_destroy(self.context, inst1['id'])
+        db.instance_destroy(self.context, inst2['id'])
+        db.instance_destroy(self.context, inst3['id'])
+        db.service_destroy(self.context, comp1['id'])
+        db.service_destroy(self.context, comp2['id'])
+
     def test_describe_instance_state(self):
         """Makes sure describe_instances for instanceState works."""
 
diff --git a/nova/tests/api/openstack/compute/test_servers.py b/nova/tests/api/openstack/compute/test_servers.py
index 2d3384dce..59d7c8a42 100644
--- a/nova/tests/api/openstack/compute/test_servers.py
+++ b/nova/tests/api/openstack/compute/test_servers.py
@@ -567,7 +567,8 @@ class ServersControllerTest(test.TestCase):
     def test_get_servers_with_bad_option(self):
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             return [fakes.stub_instance(100, uuid=server_uuid)]
 
         self.stubs.Set(nova.compute.API, 'get_all', fake_get_all)
@@ -581,7 +582,8 @@ class ServersControllerTest(test.TestCase):
     def test_get_servers_allows_image(self):
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             self.assertTrue('image' in search_opts)
             self.assertEqual(search_opts['image'], '12345')
@@ -596,7 +598,8 @@ class ServersControllerTest(test.TestCase):
         self.assertEqual(servers[0]['id'], server_uuid)
 
     def test_tenant_id_filter_converts_to_project_id_for_admin(self):
-        def fake_get_all(context, filters=None, instances=None):
+        def fake_get_all(context, filters=None, sort_key=None,
+                         sort_dir='desc'):
             self.assertNotEqual(filters, None)
             self.assertEqual(filters['project_id'], 'fake')
             self.assertFalse(filters.get('tenant_id'))
@@ -612,7 +615,8 @@ class ServersControllerTest(test.TestCase):
         self.assertTrue('servers' in res)
 
     def test_admin_restricted_tenant(self):
-        def fake_get_all(context, filters=None, instances=None):
+        def fake_get_all(context, filters=None, sort_key=None,
+                         sort_dir='desc'):
             self.assertNotEqual(filters, None)
             self.assertEqual(filters['project_id'], 'fake')
             return [fakes.stub_instance(100)]
@@ -627,7 +631,8 @@ class ServersControllerTest(test.TestCase):
         self.assertTrue('servers' in res)
 
     def test_admin_all_tenants(self):
-        def fake_get_all(context, filters=None, instances=None):
+        def fake_get_all(context, filters=None, sort_key=None,
+                         sort_dir='desc'):
             self.assertNotEqual(filters, None)
             self.assertTrue('project_id' not in filters)
             return [fakes.stub_instance(100)]
@@ -642,7 +647,8 @@ class ServersControllerTest(test.TestCase):
         self.assertTrue('servers' in res)
 
     def test_all_tenants(self):
-        def fake_get_all(context, filters=None, instances=None):
+        def fake_get_all(context, filters=None, sort_key=None,
+                         sort_dir='desc'):
             self.assertNotEqual(filters, None)
             self.assertEqual(filters['project_id'], 'fake')
             return [fakes.stub_instance(100)]
@@ -658,7 +664,8 @@ class ServersControllerTest(test.TestCase):
     def test_get_servers_allows_flavor(self):
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             self.assertTrue('flavor' in search_opts)
             # flavor is an integer ID
@@ -676,7 +683,8 @@ class ServersControllerTest(test.TestCase):
     def test_get_servers_allows_status(self):
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             self.assertTrue('vm_state' in search_opts)
             self.assertEqual(search_opts['vm_state'], vm_states.ACTIVE)
@@ -699,7 +707,8 @@ class ServersControllerTest(test.TestCase):
     def test_get_servers_allows_name(self):
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             self.assertTrue('name' in search_opts)
             self.assertEqual(search_opts['name'], 'whee.*')
@@ -716,7 +725,8 @@ class ServersControllerTest(test.TestCase):
     def test_get_servers_allows_changes_since(self):
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             self.assertTrue('changes-since' in search_opts)
             changes_since = datetime.datetime(2011, 1, 24, 17, 8, 1,
@@ -746,7 +756,8 @@ class ServersControllerTest(test.TestCase):
         """
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             # Allowed by user
             self.assertTrue('name' in search_opts)
@@ -773,7 +784,8 @@ class ServersControllerTest(test.TestCase):
         """
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             # Allowed by user
             self.assertTrue('name' in search_opts)
@@ -800,7 +812,8 @@ class ServersControllerTest(test.TestCase):
         """
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             self.assertTrue('ip' in search_opts)
             self.assertEqual(search_opts['ip'], '10\..*')
@@ -821,7 +834,8 @@ class ServersControllerTest(test.TestCase):
         """
         server_uuid = str(utils.gen_uuid())
 
-        def fake_get_all(compute_self, context, search_opts=None):
+        def fake_get_all(compute_self, context, search_opts=None,
+                         sort_key=None, sort_dir='desc'):
             self.assertNotEqual(search_opts, None)
             self.assertTrue('ip6' in search_opts)
             self.assertEqual(search_opts['ip6'], 'ffff.*')