OpenStack Compute (nova)

Code review comment for lp:~vishvananda/nova/orm_deux

orm_deux
Merge into trunk

Revision history for this message

Vish Ishaya (vishvananda) wrote on 2010-09-13:

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.

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.

« Back to merge proposal