OpenStack Compute (nova)

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

  1. ec2-filters
  2. Merge into trunk
Proposed by justinsb
Status: Work in progress
Proposed branch: lp://qastaging/~justin-fathomdb/nova/ec2-filters
Merge into: lp://qastaging/~hudson-openstack/nova/trunk
Diff against target: 417 lines (+270/-10)
7 files modified
nova/api/ec2/apirequest.py (+7/-4)
nova/api/ec2/cloud.py (+27/-2)
nova/api/predicates.py (+133/-0)
nova/db/api.py (+5/-0)
nova/db/sqlalchemy/api.py (+49/-0)
nova/tests/test_cloud.py (+39/-0)
nova/volume/api.py (+10/-4)
To merge this branch: bzr merge lp://qastaging/~justin-fathomdb/nova/ec2-filters
Reviewer Review Type Date Requested Status
Nova Core security contacts Pending
Devin Carlen Pending
Review via email: mp+53266@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2011-03-11.

Description of the change

Initial support for EC2 filters

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

To post a comment you must log in.
Revision history for this message
justinsb (justin-fathomdb) wrote : Posted in a previous version of this proposal

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...

Revision history for this message
Devin Carlen (devcamcar) wrote : Posted in a previous version of this proposal

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
Revision history for this message
justinsb (justin-fathomdb) wrote : Posted in a previous version of this proposal

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?

Revision history for this message
Thierry Carrez (ttx) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Devin Carlen (devcamcar) wrote : Posted in a previous version of this proposal

Agreed; let's bring this in for Diablo.

Revision history for this message
Devin Carlen (devcamcar) wrote : Posted in a previous version of this proposal

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

Revision history for this message
justinsb (justin-fathomdb) wrote : Posted in a previous version of this proposal

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"

Revision history for this message
Thierry Carrez (ttx) wrote :

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.

Reply
Revision history for this message
justinsb (justin-fathomdb) wrote :

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.

Reply
Revision history for this message
Devin Carlen (devcamcar) wrote :

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. :)

Reply
Revision history for this message
Thierry Carrez (ttx) wrote : Posted in a previous version of this proposal

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

Added unit test, fixed a few bugs

788. By justinsb

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

787. By justinsb

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.