Merge lp:~justin-fathomdb/nova/ec2-filters into lp:~hudson-openstack/nova/trunk

[justinsb]

Initial support for EC2 filters

Targeted for diablo, but I can't get LP to understand that!

[justinsb (justin-fathomdb)]

I could use some pointers on how to unit-test the EC2 API, so I can check this still works when I'm not cheating and going direct to the CloudController...

[Devin Carlen (devcamcar)]

Hi Justin,

Nice work here but I think it's a bit overkill for the bug. Excerpt from bug 732924:

"This makes it difficult to figure out which project a particular volume is associated with. Instances, on the other hand specifically return a project as the owner (in a separate attribute at that):

[ownerId] => testproject

Similarly, addresses also report the project as an owner:

[instanceId] => i-00000002 (testproject)"

The bug isn't that we need any advanced filtering. This only affects EC2 API and how we have to work around a few of its sharp edges.

And it looks like Ryan Lane already made this fix:

https://code.launchpad.net/~rlane/nova/lp732924/+merge/53049

So what is the need for this merge? It doesn't seem to have anything to do with the bug you linked.

review: Needs Information

[justinsb (justin-fathomdb)]

Ryan raised his issue on the mailing list and I said that this was one of the three solutions I saw, and that I was working on it. The other options were to put the projectId into the metadata, or to change the status field. To be honest, I didn't think we'd be willing to change the status field, because we've just broken anyone that was previously using that to get the user.

The fact that an alternative solution was merged does rather reduce the need for the patch, but this is definitely a solution that Ryan and I discussed to his issue, which is why I linked the bug.

This patch is still the "right thing to do", and we'll have to do it for true EC2 API compatibility, which I thought was already an approved goal. We'll probably also have to implement filtering for the OpenStack API once users have a sufficient number of VMs (which I presume is why AWS implemented it), which is why the patch is architected with a separation of the AWS filterspec from an underlying predicate. Finally, it would dramatically simplify and harmonize a lot of our code, by using functions that take predicates rather than a long list of similar functions varying only in their arguments.

But maybe not in Cactus? If we're locking down the cactus branch for release, should I propose a merge into the Diablo branch?

[Thierry Carrez (ttx)]

The referenced bug is already fixed. That sounds like a new feature we could easily rubberstamp for Diablo.

[Devin Carlen (devcamcar)]

Agreed; let's bring this in for Diablo.

[Devin Carlen (devcamcar)]

Marked as rejected for now. We'll re-review in Diablo.

[justinsb (justin-fathomdb)]

How do I propose for merge into Diablo, so that the code doesn't get lost?

Is the release process (in terms of version control branches) documented
anywhere? I'm finding it all a bit "unusual"

[Thierry Carrez (ttx)]

You should keep the branch merge proposal "work in progress" until Diablo branches open. You'll probably have to merge with trunk before reproposing for merging.

I'd suggest filing a blueprint for a review (rubberstamp) session at the summit, linking to the branch.

[justinsb (justin-fathomdb)]

That seems a highly unusual "process". Is this process documented and
agreed? (And can you send me a link please!)

I think perhaps at Diablo we should have a discussion about which of
the successful open source release/development models we should use.

[Devin Carlen (devcamcar)]

Maybe its a bit unusual but there's no diablo branch right now to propose merging into. Just means you'll have to babysit the branch for a little while. :)

[Thierry Carrez (ttx)]

Keep your branch until the Diablo merge window opens. You don't need to propose for merging. The code won't get lost. The branch will stay.

You can also set up your own "development branch" where you merge all those "feature branches" for more advanced testing.

Unmerged revisions

789. By justinsb on 2011-03-11

Added unit test, fixed a few bugs

788. By justinsb on 2011-03-11

Initial implementation of predicates, exposed as filters in the EC2 API

787. By justinsb on 2011-03-11

Cope with keys with more than one dot; don't just ignore the extra bits!

=== modified file 'nova/api/ec2/apirequest.py'
--- nova/api/ec2/apirequest.py	2011-03-04 19:01:25 +0000
+++ nova/api/ec2/apirequest.py	2011-03-14 15:58:21 +0000
@@ -133,11 +133,14 @@
                 # NOTE(vish): Automatically convert strings back
                 #             into their respective values
                 value = _try_convert(value)
+
             if len(parts) > 1:
-                d = args.get(key, {})
-                d[parts[1]] = value
-                value = d
-            args[key] = value
+                parent = args.setdefault(key, {})
+                for i in xrange(1, len(parts) - 1):
+                    parent = parent.setdefault(parts[i], {})
+                parent[parts[-1]] = value
+            else:
+                args[key] = value
 
         for key in args.keys():
             # NOTE(vish): Turn numeric dict keys into lists
 
=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-03-03 23:05:00 +0000
+++ nova/api/ec2/cloud.py	2011-03-14 15:58:21 +0000
@@ -2,6 +2,7 @@
 
 # Copyright 2010 United States Government as represented by the
 # Administrator of the National Aeronautics and Space Administration.
+# Copyright 2011 Justin Santa Barbara
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -39,6 +40,7 @@
 from nova import network
 from nova import utils
 from nova import volume
+from nova.api import predicates
 from nova.api.ec2 import ec2utils
 from nova.compute import instance_types
 from nova.image import s3
@@ -533,15 +535,38 @@
         return self.compute_api.get_ajax_console(context,
                                                  instance_id=instance_id)
 
-    def describe_volumes(self, context, volume_id=None, **kwargs):
+    def _map_ec2_filter_to_predicate(self, ec2_filters):
+        if ec2_filters is None:
+            return predicates.TruePredicate()
+        value_predicates = []
+        for ec2_filter in ec2_filters:
+            name = ec2_filter['name']
+            acceptable_values = []
+            for v in ec2_filter['value'].values():
+                acceptable_values.append(v)
+            if not acceptable_values:
+                return predicates.FalsePredicate()
+            value_predicates.append(predicates.ValuePredicate(
+                                                        name,
+                                                        acceptable_values))
+        if not value_predicates:
+            return predicates.TruePredicate()
+
+        return predicates.CompoundAndPredicate(value_predicates)
+
+    def describe_volumes(self, context, volume_id=None, filter=None,
+                         **kwargs):
+        predicate = self._map_ec2_filter_to_predicate(filter)
         if volume_id:
+            #TODO(justinsb): We probably should support filters here
+            #  but filters with specified ids are a very odd request
             volumes = []
             for ec2_id in volume_id:
                 internal_id = ec2utils.ec2_id_to_id(ec2_id)
                 volume = self.volume_api.get(context, internal_id)
                 volumes.append(volume)
         else:
-            volumes = self.volume_api.get_all(context)
+            volumes = self.volume_api.get_all(context, predicate)
         volumes = [self._format_volume(context, v) for v in volumes]
         return {'volumeSet': volumes}
 
 
=== added file 'nova/api/predicates.py'
--- nova/api/predicates.py	1970-01-01 00:00:00 +0000
+++ nova/api/predicates.py	2011-03-14 15:58:21 +0000
@@ -0,0 +1,133 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Justin Santa Barbara
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""Model classes that represent predicates"""
+
+from nova import log as logging
+
+
+LOG = logging.getLogger('nova.api.predicates')
+
+# Well known properties
+PROPERTY_PROJECT_ID = 'project_id'
+
+
+class Predicate(object):
+    """A predicate is a logical function which is either true or false.
+
+    We use it for filtering values"""
+    def __init__(self):
+        pass
+
+    def join_and(self, rhs):
+        """Returns a predicate which is true iff both this and rhs are true"""
+        return CompoundAndPredicate([self, rhs])
+
+    def filter_by_project(self, project_id):
+        """Adds a predicate so we only match the specified project"""
+        project_filter = ValuePredicate(PROPERTY_PROJECT_ID, project_id)
+        return self.join_and(project_filter)
+
+    def collapse_to_filters(self):
+        """Collapse to a dictionary of key => value_list filters.
+
+        If the predicate is unsatisfiable, return False.
+        If the predicate cannot be simplified in this way, raise an exception.
+        The predicate should be true for an object o iff
+        for each k,v in map: o[k] in v
+        """
+        raise NotImplementedError()
+
+
+class ValuePredicate(Predicate):
+    """A predicate on a single property
+
+    The predicate is satisfied if the value is a member of acceptable_values
+    """
+    def __init__(self, key, acceptable_values):
+        super(ValuePredicate, self).__init__()
+        self.key = key
+        if not isinstance(acceptable_values, list):
+            raise Exception("acceptable_values must be a list")
+        self.acceptable_values = acceptable_values
+
+    def collapse_to_filters(self):
+        collapsed = {}
+        collapsed[self.key] = self.acceptable_values
+        return collapsed
+
+
+def _intersect(lhs, rhs):
+    """Returns the list of items in both lhs and rhs"""
+    return list(set(lhs) & set(rhs))
+
+
+class CompoundAndPredicate(Predicate):
+    """A predicate where all children must be satisfied"""
+    def __init__(self, children):
+        super(CompoundAndPredicate, self).__init__()
+        self.children = children
+
+    def join_and(self, rhs):
+        #NOTE(justinsb): We could do some immediate-simplification here
+        #  e.g. if rhs is a CompoundAndPredicate
+        conjunction = self.children + rhs
+        return CompoundAndPredicate(conjunction)
+
+    def collapse_to_filters(self):
+        collapsed = {}
+        for child in self.children:
+            child_map = child.collapse_to_filters()
+            if child_map == False:
+                return False
+
+            for k, v in child_map.iteritems():
+                values = collapsed.get(k)
+                if values is None:
+                    values = v
+                else:
+                    values = _intersect(values, v)
+                    if not values:
+                        # Unsatisfiable (something like X=A and X=B, A != B)
+                        return False
+
+                collapsed[k] = values
+
+        return collapsed
+
+
+class FalsePredicate(Predicate):
+    """A predicate which is always False"""
+
+    def join_and(self, rhs):
+        # We are never true, so our conjunction can never be true
+        return FalsePredicate()
+
+    def collapse_to_filters(self):
+        # Unsatisfiable, return False
+        return False
+
+
+class TruePredicate(Predicate):
+    """A predicate which is always True"""
+
+    def join_and(self, rhs):
+        return rhs
+
+    def collapse_to_filters(self):
+        # No criteria
+        return {}
 
=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-03-09 21:27:38 +0000
+++ nova/db/api.py	2011-03-14 15:58:21 +0000
@@ -770,6 +770,11 @@
     return IMPL.volume_get_all_by_project(context, project_id)
 
 
+def volume_get_predicated(context, predicate):
+    """Get a list of volumes matching the specified predicate."""
+    return IMPL.volume_get_predicated(context, predicate)
+
+
 def volume_get_by_ec2_id(context, ec2_id):
     """Get a volume by ec2 id."""
     return IMPL.volume_get_by_ec2_id(context, ec2_id)
 
=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-03-11 23:05:02 +0000
+++ nova/db/sqlalchemy/api.py	2011-03-14 15:58:21 +0000
@@ -2,6 +2,7 @@
 
 # Copyright 2010 United States Government as represented by the
 # Administrator of the National Aeronautics and Space Administration.
+# Copyright 2011 Justin Santa Barbara
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -25,7 +26,9 @@
 from nova import db
 from nova import exception
 from nova import flags
+from nova import log as logging
 from nova import utils
+from nova.api import predicates
 from nova.db.sqlalchemy import models
 from nova.db.sqlalchemy.session import get_session
 from sqlalchemy import or_
@@ -36,8 +39,11 @@
 from sqlalchemy.sql import func
 from sqlalchemy.sql.expression import literal_column
 
+
 FLAGS = flags.FLAGS
 
+LOG = logging.getLogger('nova.db.sqlalchemy')
+
 
 def is_admin_context(context):
     """Indicates if the request context is an administrator."""
@@ -1511,6 +1517,49 @@
     return result
 
 
+@require_context
+def volume_get_predicated(context, predicate=None):
+    session = get_session()
+    query = session.query(models.Volume).\
+                   options(joinedload('instance')).\
+                   filter_by(deleted=can_read_deleted(context))
+
+    if predicate:
+        filters = predicate.collapse_to_filters()
+        LOG.debug("filters=%s", filters)
+    else:
+        filters = {}
+
+    # If the predicate is known-unsatisfiable, don't bother hitting the DB
+    if filters == False:
+        return []
+
+    filtered_project_id = None
+
+    # For now, we whitelist/hard-code the filters...
+    for k, v in filters.iteritems():
+        if k == predicates.PROPERTY_PROJECT_ID:
+            for project_id in v:
+                authorize_project_context(context, project_id)
+
+            #TODO(justinsb): Does SA simplify a single-item IN clause?
+            if len(v) == 1:
+                query = query.filter(models.Volume.project_id == v[0])
+            else:
+                query = query.filter(models.Volume.project_id.in_(v))
+
+            filtered_project_id = v
+        else:
+            raise exception.ApiError(_("Unknown predicate: %s") % k)
+
+    # Only an admin can do a query without a project filter
+    if not filtered_project_id:
+        if not is_admin_context(context):
+            raise exception.NotAuthorized()
+
+    return query.all()
+
+
 @require_admin_context
 def volume_get_all(context):
     session = get_session()
 
=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-03-10 04:42:11 +0000
+++ nova/tests/test_cloud.py	2011-03-14 15:58:21 +0000
@@ -30,6 +30,7 @@
 from nova import context
 from nova import crypto
 from nova import db
+from nova import exception
 from nova import flags
 from nova import log as logging
 from nova import rpc
@@ -176,6 +177,44 @@
         db.volume_destroy(self.context, vol1['id'])
         db.volume_destroy(self.context, vol2['id'])
 
+    def test_describe_volumes_filters(self):
+        """Makes sure describe_volumes works and filters results."""
+        vol1 = db.volume_create(self.context, {'project_id': 'proj1'})
+        vol2 = db.volume_create(self.context, {'project_id': 'proj2'})
+        
+        filtr = [{ 'name': 'project_id', 'value': {'1': 'proj1'} }]      
+        result = self.cloud.describe_volumes(self.context, filter=filtr)
+        self.assertEqual(len(result['volumeSet']), 1)
+
+        filtr = [{ 'name': 'project_id', 'value': {'1': 'proj2'} }]      
+        result = self.cloud.describe_volumes(self.context, filter=filtr)
+        self.assertEqual(len(result['volumeSet']), 1)
+
+        filtr = [{ 'name': 'project_id', 'value': {'1': 'proj3'} }]      
+        result = self.cloud.describe_volumes(self.context, filter=filtr)
+        self.assertEqual(len(result['volumeSet']), 0)
+
+        filtr = [{ 'name': 'project_id', 'value': {'1': 'proj1',
+                                                   '2': 'proj2'} }]      
+        result = self.cloud.describe_volumes(self.context, filter=filtr)
+        self.assertEqual(len(result['volumeSet']), 2)
+
+        # Unsatisfiable (shouldn't even hit DB!)
+        filtr = [{ 'name': 'project_id', 'value': {'1': 'proj1'} },
+                 { 'name': 'project_id', 'value': {'1': 'proj2'} },
+                ]
+        result = self.cloud.describe_volumes(self.context, filter=filtr)
+        self.assertEqual(len(result['volumeSet']), 0)
+
+        # Not a supported value
+        filtr = [{ 'name': 'notso_key', 'value': {'1': 'notso_value'} }]
+        self.assertRaises(exception.ApiError,
+                          self.cloud.describe_volumes,
+                          self.context, filter=filtr)
+
+        db.volume_destroy(self.context, vol1['id'])
+        db.volume_destroy(self.context, vol2['id'])
+
     def test_describe_availability_zones(self):
         """Makes sure describe_availability_zones works and filters results."""
         service1 = db.service_create(self.context, {'host': 'host1_zones',
 
=== modified file 'nova/volume/api.py'
--- nova/volume/api.py	2011-02-19 07:15:42 +0000
+++ nova/volume/api.py	2011-03-14 15:58:21 +0000
@@ -2,6 +2,7 @@
 
 # Copyright 2010 United States Government as represented by the
 # Administrator of the National Aeronautics and Space Administration.
+# Copyright 2011 Justin Santa Barbara
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -28,6 +29,7 @@
 from nova import log as logging
 from nova import quota
 from nova import rpc
+from nova.api import predicates
 from nova.db import base
 
 FLAGS = flags.FLAGS
@@ -84,10 +86,14 @@
     def get(self, context, volume_id):
         return self.db.volume_get(context, volume_id)
 
-    def get_all(self, context):
-        if context.is_admin:
-            return self.db.volume_get_all(context)
-        return self.db.volume_get_all_by_project(context, context.project_id)
+    def get_all(self, context, predicate=None):
+        if predicate is None:
+            predicate = predicates.TruePredicate()
+
+        if not context.is_admin:
+            predicate = predicate.filter_by_project(context.project_id)
+
+        return self.db.volume_get_predicated(context, predicate)
 
     def check_attach(self, context, volume_id):
         volume = self.get(context, volume_id)