Sort results from describe_instances in EC2 API.

Fix bug 827619.

This bug pointed out that EC2 sorts the results of of describe_instances
by the launch time.  Make our implementation of the EC2 API behave the same
way.  Previously, instances coming out of the db API were sorted by the
key we wanted, but in the opposite order.  You can now specify both a
sort key and order.  The behavior is the same by default, but the EC2
API sets the parameter to do an ascending sort.

Change-Id: Ifd0bc79ad4c4c8c45809dbb1ac2dadf8abcfd4c3

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.*')