Code review comment for 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.

« Back to merge proposal