Merge lp:~vishvananda/nova/orm_deux into lp:~hudson-openstack/nova/trunk

Proposed by Vish Ishaya
Status: Merged
Approved by: Soren Hansen
Approved revision: 379
Merge reported by: Vish Ishaya
Merged at revision: not available
Proposed branch: lp:~vishvananda/nova/orm_deux
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 21 lines (+2/-2)
1 file modified
nova/endpoint/cloud.py (+2/-2)
To merge this branch: bzr merge lp:~vishvananda/nova/orm_deux
Reviewer Review Type Date Requested Status
Jay Pipes (community) Approve
Eric Day (community) Approve
Soren Hansen (community) Needs Information
Review via email: mp+34263@code.launchpad.net

Description of the change

Proposing merge to get feedback on orm refactoring. I am very interested in feedback to all of these changes.

This is a huge set of changes, that touches almost all of the files. I'm sure I have broken quite a bit, but better to take the plunge now than to postpone this until later. The idea is to allow for pluggable backends throughout the code.

Brief Overview
For compute/volume/network, there are multiple classes
service - responsible for rpc
  this currently uses the existing cast and call in rpc.py and a little bit of magic
  to call public methods on the manager class.
  each service also reports its state into the database every 10 seconds
manager - responsible for managing respective object classes
  all the business logic for the classes go here
db (db_driver) - responsible for abstracting database access
driver (domain_driver) - responsible for executing actual shell commands and implementation

Compute hasn't been fully cleaned up, but to get an idea of how it works, take a look
at volume and network

Known issues/Things to be done:

* nova-api accesses db objects directly
  It seems cleaner to have only the managers dealing with their respective objects. This would
  mean code for 'run_instances' would move into the manager class and it would do the initial
  setup and cast out to the remote service

* db code uses flat methods to define its interface
  In my mind this is a little prettier as an abstract base class, but driver loading code
  can load a module or a class. It works, so I'm not sure it needs to be changed but feel
  free to debate it.

* Service classes have no code in them
  Not sure if this is a problem for people, but the magic of calling the manager's methods is
  done in the base class. We could remove the magic from the base class and explicitly
  wrap methods that we want to make available via rpc if this seems nasty.

* AuthManager Projects/Users/Roles are not integrated into this system.
  In order for everything to live happily in the backend, we need some type
  of adaptor for LDAP

* Context is not passed properly across rabbit
  Context should probably be changed to a simple dictionary so that it can be
  passed properly through the queue

* No authorization checks on access to objects
  We need to decide on which layer auth checks should happen.

* Some of the methods in ComputeManager need to be moved into other layers/managers
* Compute driver layer should be abstracted more cleanly
* Flat networking is untested and may need to be reworked
* Some of the api commands are not working yet
* Nova Swift Authentication needs to be refactored(Todd is working on this)

To post a comment you must log in.
Revision history for this message
Eric Day (eday) wrote :

Here are my notes and questions from the first pass through all this:

What is context used for in all the DB calls? Is it really needed? If it is, I think we should create instances of various objects and set it as a class instance member, not passed through to every method.

Why do we have services and managers? Seems like services should just be removed, or move manager->service. If we need a service wrapper around the manager, we could pass a manager class into nova/service.py directly to create the service, removing nova/{compute,network,...}/service.py files entirely.

nova/datastore.old.py
Why is this still here?

nova/db/api.py
This should either be an interface class that IMPL overrides or not even have an API class and just document what methods an IMPL needs to provide.

nova/db/sqlalchemy/api.py
Again, class with methods (context being an instance member) would be cleaner. A
lso, some methods accept _context but then pass them through to other methods. T
he underscore should be removed in these cases.

nova/db/sqlalchemy/models.py
Why doesn't NovaBase inherit from BASE, instead of all models?

nova/virt/libvirt_conn.py
Text conflict in nova/virt/libvirt_conn.py, re-merge with trunk.

run_tests.py
Replace/remove nova.tests.model_unittest?

review: Needs Information
Revision history for this message
Vish Ishaya (vishvananda) wrote :

See Below:

On Wed, Sep 1, 2010 at 5:15 PM, Eric Day <email address hidden> wrote:

> Review: Needs Information
> Here are my notes and questions from the first pass through all this:
>
> What is context used for in all the DB calls? Is it really needed? If it
> is, I think we should create instances of various objects and set it as a
> class instance member, not passed through to every method.
>
> Why do we have services and managers? Seems like services should just be
> removed, or move manager->service. If we need a service wrapper around the
> manager, we could pass a manager class into nova/service.py directly to
> create the service, removing nova/{compute,network,...}/service.py files
> entirely.
>

Good point. The services were to allow for overriding on specific rpc
calls, but we can always add in specific services if we ever actually do
need to override them. I've removed them for now.

>
> nova/datastore.old.py
> Why is this still here?
>

deleted.

>
> nova/db/api.py
> This should either be an interface class that IMPL overrides or not even
> have an API class and just document what methods an IMPL needs to provide.
>

I'm fine with this, but I don't think it should be a blocker for merge.
 Some people from are team really prefer flat module based interfaces vs.
classes. I tend to prefer classes, but it isn't a big deal

>
> nova/db/sqlalchemy/api.py
> Again, class with methods (context being an instance member) would be
> cleaner. A
> lso, some methods accept _context but then pass them through to other
> methods. T
> he underscore should be removed in these cases.
>

I fixed the _context references. Again I'm happy for the class interface if
people have a strong preference

>
> nova/db/sqlalchemy/models.py
> Why doesn't NovaBase inherit from BASE, instead of all models?
>

__table__ or __tablename__ needs to be defined on all sqlalchemy models that
inherit from Base. Since NovaBase just contains common base code it doesn't
have one. If anyone knows a cleaner way to do this, I'm all ears. I am
definitely not a sqlalchemy expert :)

>
> nova/virt/libvirt_conn.py
> Text conflict in nova/virt/libvirt_conn.py, re-merge with trunk.
>

remerged

>
> run_tests.py
> Replace/remove nova.tests.model_unittest?
>

gone. Todd is rewriting the tests for auth which were in here. Some data
layer tests would be useful as well

> --
> https://code.launchpad.net/~vishvananda/nova/orm_deux/+merge/34263
> You are the owner of lp:~vishvananda/nova/orm_deux.
>

Revision history for this message
Soren Hansen (soren) wrote :

Perhaps it's an artefact from SQLAlchemy (which I've not used before, so bear with me :) ), but I'm wondering why {service,fixed_ip,instance,network,network_index,export_device,volume}_create take a "values" argument instead of just regular keyword arguments? Also, why don't floating_ip_create and network_create_fixed_ips follow the same pattern?

review: Needs Information
Revision history for this message
termie (termie) wrote :

Re: Soren, why they take a values object.

There were already even in just making these classes instances where the values we were updating included 'instance_id' and the like, having those as kwargs resulted in duplicate values when using positional and keyword args. In the best case we would write out all the paremters that can be changed in the function signature and document them all but that seemed like a lot to ask at this point.

Re: Eric, context as a param to a function vs as an instance attribute in a method. There are a few reasons.

1. Context can be different from call to call as context is intended to include the permissions of the role who is making the call. An example of this would be us having to make background bookkeeping changes when a user changes something they own, the user is allowed to make the call for the initial change but the bookkeeping may require higher privileges so a different context would be passed to it.

2. Functional interfaces are significantly easier to test than OO ones that include state.

3. If you don't have the context param there, something like:

db.user_get(context, blah)

will usually become:

db = Db.fromContext(context)
db.user_get(blah)

So even in the case where the context would be static throughout the calls, you still end up instantiating new db objects in every method down the chain or passing along 'db' keyword args that do the same thing as context. In most cases we are only making one or two calls with the db so the extra boilerplate becomes more trouble than it is worth.

Revision history for this message
termie (termie) wrote :

make that first line above be:

There were already, even in just making these classes, instances where

Revision history for this message
Vish Ishaya (vishvananda) wrote :

Soren: Not following the standard pattern was simply a change that i missed in my many revisions. I've fixed everything to use the same pattern.

Termie: I believe that the duplicates are gone now, but I'm not totally happy with the handling of str_id in the data layer. In any case, we could change them to use kwargs instead if that is a preferable syntax.

Vish
On Sep 8, 2010, at 7:39 AM, termie wrote:

> Re: Soren, why they take a values object.
>
> There were already even in just making these classes instances where the values we were updating included 'instance_id' and the like, having those as kwargs resulted in duplicate values when using positional and keyword args. In the best case we would write out all the paremters that can be changed in the function signature and document them all but that seemed like a lot to ask at this point.
>
> Re: Eric, context as a param to a function vs as an instance attribute in a method. There are a few reasons.
>
> 1. Context can be different from call to call as context is intended to include the permissions of the role who is making the call. An example of this would be us having to make background bookkeeping changes when a user changes something they own, the user is allowed to make the call for the initial change but the bookkeeping may require higher privileges so a different context would be passed to it.
>
> 2. Functional interfaces are significantly easier to test than OO ones that include state.
>
> 3. If you don't have the context param there, something like:
>
> db.user_get(context, blah)
>
> will usually become:
>
> db = Db.fromContext(context)
> db.user_get(blah)
>
> So even in the case where the context would be static throughout the calls, you still end up instantiating new db objects in every method down the chain or passing along 'db' keyword args that do the same thing as context. In most cases we are only making one or two calls with the db so the extra boilerplate becomes more trouble than it is worth.
>
>
> --
> https://code.launchpad.net/~vishvananda/nova/orm_deux/+merge/34263
> You are the owner of lp:~vishvananda/nova/orm_deux.

Revision history for this message
Soren Hansen (soren) wrote :

On 08-09-2010 18:31, vishvananda wrote:
> Soren: Not following the standard pattern was simply a change that i
> missed in my many revisions. I've fixed everything to use the same
> pattern.

Ok.

> Termie: I believe that the duplicates are gone now, but I'm not
> totally happy with the handling of str_id in the data layer. In any
> case, we could change them to use kwargs instead if that is a
> preferable syntax.

At least for me, I think the kwargs approach would seem less foreign.

--
Soren Hansen
Ubuntu Developer
http://www.ubuntu.com/

Revision history for this message
Eric Day (eday) wrote :

That sounds fine on the functional vs oo design. I would still prefer the OO design with the context member init and change during usage since this removes passing it through all the other functions. Either way I'm fine, and certainly don't want this to hold up the branch anymore given it's size and scope.

I propose we get this merged soon so we don't have as many diverging branches.

review: Approve
Revision history for this message
Jay Pipes (jaypipes) wrote :

Yo!

Overall, I do like the new interface. I agree with Eric that an object-oriented approach would be nice, but I certainly wouldn't disapprove this because of that. :)

Some tiny nits to note and deal with at some later point:

* redis_db flag should be removed
* I recommend changing the name of the db_backend flag to db_driver to match other flags (like compute_driver
* Typos:
 s/Represates/Represents/g
 s/Rignt/Right/g
* For these new flags:

4174 +DEFINE_string('compute_manager', 'nova.compute.manager.ComputeManager',
4175 + 'Manager for compute')
4176 +DEFINE_string('network_manager', 'nova.network.manager.VlanManager',
4177 + 'Manager for network')
4178 +DEFINE_string('volume_manager', 'nova.volume.manager.AOEManager',
4179 + 'Manager for volume')

I'm wondering why the need for those? Aren't the manager classes fixed and become adaptable via their internal drivers? For instance, the compute manager uses the flag compute_driver for its adaptation, no?

Or am I missing something?

Cheers!
jay

review: Approve
Revision history for this message
Vish Ishaya (vishvananda) wrote :

I'm not sure all differences can be contained in the driver. For example, in the network code, the driver is the code responsible for actually setting up forwarding rules. Linux_net sets up forwarding rules, bridges, vlans, etc. for linux boxes, but there may be a cisco_net or some such to actually run commands on a router for example. These changes are orthogonal to which networking strategy to use: i.e. Flat networking vs Vlan networking.

We may have a similar issue for volumes: It would be great if the changes necessary for ISCSi can be encapsulated at the driver level, but it may not be possible. The idea of the flags is that we are pluggable at the manager level for strategy changes and at the driver level for implementation changes.

Redis db is still used for fakeLdap, at least until someone feels like writing one that works with mysql.

I'll fix those typos in a bit.

Vish

On Sep 13, 2010, at 8:08 AM, Jay Pipes wrote:

> Review: Approve
> Yo!
>
> Overall, I do like the new interface. I agree with Eric that an object-oriented approach would be nice, but I certainly wouldn't disapprove this because of that. :)
>
> Some tiny nits to note and deal with at some later point:
>
> * redis_db flag should be removed
> * I recommend changing the name of the db_backend flag to db_driver to match other flags (like compute_driver
> * Typos:
> s/Represates/Represents/g
> s/Rignt/Right/g
> * For these new flags:
>
> 4174 +DEFINE_string('compute_manager', 'nova.compute.manager.ComputeManager',
> 4175 + 'Manager for compute')
> 4176 +DEFINE_string('network_manager', 'nova.network.manager.VlanManager',
> 4177 + 'Manager for network')
> 4178 +DEFINE_string('volume_manager', 'nova.volume.manager.AOEManager',
> 4179 + 'Manager for volume')
>
> I'm wondering why the need for those? Aren't the manager classes fixed and become adaptable via their internal drivers? For instance, the compute manager uses the flag compute_driver for its adaptation, no?
>
> Or am I missing something?
>
> Cheers!
> jay
>
> --
> https://code.launchpad.net/~vishvananda/nova/orm_deux/+merge/34263
> You are the owner of lp:~vishvananda/nova/orm_deux.

Revision history for this message
Jay Pipes (jaypipes) wrote :

> I'm not sure all differences can be contained in the driver. For example, in
> the network code, the driver is the code responsible for actually setting up
> forwarding rules. Linux_net sets up forwarding rules, bridges, vlans, etc.
> for linux boxes, but there may be a cisco_net or some such to actually run
> commands on a router for example. These changes are orthogonal to which
> networking strategy to use: i.e. Flat networking vs Vlan networking.

Ah, I see. OK, makes sense then.

> Redis db is still used for fakeLdap, at least until someone feels like writing
> one that works with mysql.

Hmm, ok, yes, I remember that code... To be removed later, then :)

> I'll fix those typos in a bit.

Cool :)
-jay

> Vish
>
> On Sep 13, 2010, at 8:08 AM, Jay Pipes wrote:
>
> > Review: Approve
> > Yo!
> >
> > Overall, I do like the new interface. I agree with Eric that an object-
> oriented approach would be nice, but I certainly wouldn't disapprove this
> because of that. :)
> >
> > Some tiny nits to note and deal with at some later point:
> >
> > * redis_db flag should be removed
> > * I recommend changing the name of the db_backend flag to db_driver to match
> other flags (like compute_driver
> > * Typos:
> > s/Represates/Represents/g
> > s/Rignt/Right/g
> > * For these new flags:
> >
> > 4174 +DEFINE_string('compute_manager',
> 'nova.compute.manager.ComputeManager',
> > 4175 + 'Manager for compute')
> > 4176 +DEFINE_string('network_manager', 'nova.network.manager.VlanManager',
> > 4177 + 'Manager for network')
> > 4178 +DEFINE_string('volume_manager', 'nova.volume.manager.AOEManager',
> > 4179 + 'Manager for volume')
> >
> > I'm wondering why the need for those? Aren't the manager classes fixed and
> become adaptable via their internal drivers? For instance, the compute
> manager uses the flag compute_driver for its adaptation, no?
> >
> > Or am I missing something?
> >
> > Cheers!
> > jay
> >
> > --
> > https://code.launchpad.net/~vishvananda/nova/orm_deux/+merge/34263
> > You are the owner of lp:~vishvananda/nova/orm_deux.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Attempt to merge lp:~vishvananda/nova/orm_deux into lp:nova failed due to merge conflicts:

contents conflict in nova/compute/service.py
text conflict in nova/tests/cloud_unittest.py
text conflict in nova/tests/volume_unittest.py

Revision history for this message
Vish Ishaya (vishvananda) wrote :

Fixed merge conflicts if someone feels like hitting approve again.

On Sep 14, 2010 3:02 PM, "OpenStack Hudson" <email address hidden> wrote:
> The proposal to merge lp:~vishvananda/nova/orm_deux into lp:nova has been
updated.
>
> Status: Approved => Needs review
> --
> https://code.launchpad.net/~vishvananda/nova/orm_deux/+merge/34263
> You are the owner of lp:~vishvananda/nova/orm_deux.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :
Download full text (52.5 KiB)

The attempt to merge lp:~vishvananda/nova/orm_deux into lp:nova failed.Below is the output from the failed tests.

nova.tests.access_unittest
  AccessTestCase
    test_001_allow_all ... [ERROR]
    test_002_allow_none ... [ERROR]
    test_003_allow_project_manager ... [ERROR]
    test_004_allow_sys_and_net ... [ERROR]
    test_005_allow_sys_no_pm ... [ERROR]
nova.tests.api_unittest
  ApiEc2TestCase
    test_describe_instances ... [ERROR]
                                        [ERROR]
    test_get_all_key_pairs ... [ERROR]
                                         [ERROR]
nova.tests.auth_unittest
  AuthTestCase
    test_001_can_create_users ... [OK]
    test_002_can_get_user ... [OK]
    test_003_can_retreive_properties ... [OK]
    test_004_signature_is_valid ... [OK]
    test_005_can_get_credentials ... [OK]
    test_006_test_key_storage ... [OK]
    test_007_test_key_generation ... [OK]
    test_008_can_list_key_pairs ... [OK]
    test_009_can_delete_key_pair ... [OK]
    test_010_can_list_users ... [OK]
    test_101_can_add_user_role ... [OK]
    test_199_can_remove_user_role ... [OK]
    test_201_can_create_project ... [ERROR]
    test_202_user1_is_project_member ... [ERROR]
    test_203_user2_is_not_project_member ... [ERROR]
    test_204_user1_is_project_manager ... [ERROR]
    test_205_user2_is_not_project_manager ... [ERROR]
    test_206_can_add_user_to_project ... [ERROR]
    test_207_can_remove_user_from_project ... [ERROR]
    test_208_can_remove_add_user_with_role ... [ERROR]
    test_209_can_generate_x509 ... [OK]
    test_210_can_add_project_role ... [ERROR]
    test_211_can_list_project_roles ... [FAIL]
    test_212_can_remove_project_role ... [ERROR]
    test_214_can_retrieve_project_by_user ... [ERROR]
    test_299_can_delete_project ... [ERROR]
    test_999_can_delete_users ... [OK]
nova.tests.cloud_unittest
  CloudTestCase
    test_console_output ... [ERROR]
    test_instance_upd...

Revision history for this message
Vish Ishaya (vishvananda) wrote :

Oh. Someone needs to add sqlalchemy to the build server.

On Sep 14, 2010 4:26 PM, "OpenStack Hudson" <email address hidden> wrote:
> The proposal to merge lp:~vishvananda/nova/orm_deux into lp:nova has been
updated.
>
> Status: Approved => Needs review
> --
> https://code.launchpad.net/~vishvananda/nova/orm_deux/+merge/34263
> You are the owner of lp:~vishvananda/nova/orm_deux.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :
Download full text (52.5 KiB)

The attempt to merge lp:~vishvananda/nova/orm_deux into lp:nova failed.Below is the output from the failed tests.

nova.tests.access_unittest
  AccessTestCase
    test_001_allow_all ... [ERROR]
    test_002_allow_none ... [ERROR]
    test_003_allow_project_manager ... [ERROR]
    test_004_allow_sys_and_net ... [ERROR]
    test_005_allow_sys_no_pm ... [ERROR]
nova.tests.api_unittest
  ApiEc2TestCase
    test_describe_instances ... [ERROR]
                                        [ERROR]
    test_get_all_key_pairs ... [ERROR]
                                         [ERROR]
nova.tests.auth_unittest
  AuthTestCase
    test_001_can_create_users ... [OK]
    test_002_can_get_user ... [OK]
    test_003_can_retreive_properties ... [OK]
    test_004_signature_is_valid ... [OK]
    test_005_can_get_credentials ... [OK]
    test_006_test_key_storage ... [OK]
    test_007_test_key_generation ... [OK]
    test_008_can_list_key_pairs ... [OK]
    test_009_can_delete_key_pair ... [OK]
    test_010_can_list_users ... [OK]
    test_101_can_add_user_role ... [OK]
    test_199_can_remove_user_role ... [OK]
    test_201_can_create_project ... [ERROR]
    test_202_user1_is_project_member ... [ERROR]
    test_203_user2_is_not_project_member ... [ERROR]
    test_204_user1_is_project_manager ... [ERROR]
    test_205_user2_is_not_project_manager ... [ERROR]
    test_206_can_add_user_to_project ... [ERROR]
    test_207_can_remove_user_from_project ... [ERROR]
    test_208_can_remove_add_user_with_role ... [ERROR]
    test_209_can_generate_x509 ... [OK]
    test_210_can_add_project_role ... [ERROR]
    test_211_can_list_project_roles ... [FAIL]
    test_212_can_remove_project_role ... [ERROR]
    test_214_can_retrieve_project_by_user ... [ERROR]
    test_299_can_delete_project ... [ERROR]
    test_999_can_delete_users ... [OK]
nova.tests.cloud_unittest
  CloudTestCase
    test_console_output ... [ERROR]
    test_instance_upd...

Revision history for this message
Vish Ishaya (vishvananda) wrote :
Download full text (53.9 KiB)

cannot import name relationship. Different versions of sqlalchemy? My pip freeze shows:

SQLAlchemy==0.6.3

Vish

On Sep 15, 2010, at 10:36 AM, OpenStack Hudson wrote:

> The attempt to merge lp:~vishvananda/nova/orm_deux into lp:nova failed.Below is the output from the failed tests.
>
> nova.tests.access_unittest
> AccessTestCase
> test_001_allow_all ... [ERROR]
> test_002_allow_none ... [ERROR]
> test_003_allow_project_manager ... [ERROR]
> test_004_allow_sys_and_net ... [ERROR]
> test_005_allow_sys_no_pm ... [ERROR]
> nova.tests.api_unittest
> ApiEc2TestCase
> test_describe_instances ... [ERROR]
> [ERROR]
> test_get_all_key_pairs ... [ERROR]
> [ERROR]
> nova.tests.auth_unittest
> AuthTestCase
> test_001_can_create_users ... [OK]
> test_002_can_get_user ... [OK]
> test_003_can_retreive_properties ... [OK]
> test_004_signature_is_valid ... [OK]
> test_005_can_get_credentials ... [OK]
> test_006_test_key_storage ... [OK]
> test_007_test_key_generation ... [OK]
> test_008_can_list_key_pairs ... [OK]
> test_009_can_delete_key_pair ... [OK]
> test_010_can_list_users ... [OK]
> test_101_can_add_user_role ... [OK]
> test_199_can_remove_user_role ... [OK]
> test_201_can_create_project ... [ERROR]
> test_202_user1_is_project_member ... [ERROR]
> test_203_user2_is_not_project_member ... [ERROR]
> test_204_user1_is_project_manager ... [ERROR]
> test_205_user2_is_not_project_manager ... [ERROR]
> test_206_can_add_user_to_project ... [ERROR]
> test_207_can_remove_user_from_project ... [ERROR]
> test_208_can_remove_add_user_with_role ... [ERROR]
> test_209_can_generate_x509 ... [OK]
> test_210_can_add_project_role ... [ERROR]
> test_211_can_list_project_roles ... [FAIL]
> test_212_can_remove_project_role ... [ERROR]
> test_214_can_retrieve_project_by_user ... [ERROR]
> test_299_can_delete_project ... [ERROR]
> ...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/endpoint/cloud.py'
2--- nova/endpoint/cloud.py 2010-09-11 11:01:44 +0000
3+++ nova/endpoint/cloud.py 2010-09-15 23:04:40 +0000
4@@ -366,7 +366,7 @@
5 instances = db.instance_get_by_reservation(context,
6 reservation_id)
7 else:
8- if not context.user.is_admin():
9+ if context.user.is_admin():
10 instances = db.instance_get_all(context)
11 else:
12 instances = db.instance_get_by_project(context,
13@@ -613,7 +613,7 @@
14 # NOTE(vish): Currently, nothing needs to be done on the
15 # network node until release. If this changes,
16 # we will need to cast here.
17- self.network.deallocate_fixed_ip(context, address)
18+ self.network_manager.deallocate_fixed_ip(context, address)
19
20 host = instance_ref['host']
21 if host: