Code review comment for lp:~cisco-openstack/neutron/l2network-plugin-persistence

> > (Relatively) Major comments:
> >
> > 1 === added file 'quantum/common/test_lib.py'
> > 2 --- quantum/common/test_lib.py 1970-01-01 00:00:00 +0000
> > 3 +++ quantum/common/test_lib.py 2011-08-18 18:13:27 +0000
> >
> > This is strange. quantum/common/test_lib.py is already in trunk, and it
> seems
> > it is identical to the one in this branch.
> >
> This file wasn't touched at all in this branch. I have made a checkin based on
> some of the changes proposed below. I'll try to sync this branch with top of
> quantum and see if this diff file goes away.

It seems the problem disappeared with your latest push.

Rohit: Yes, a new test_lib.py seems to have been added in one of the revisions on lp:quantum. So, we had to ensure that the cisco branch had that new file to avoid the conflict.

>
> > 4168 -4323: These are unit tests specific for quantum.db. I'd propose to
> have
> > them in a separate file in /tests/unit. Thank you very much for implementing
> > these much-needed unit tests!!!
> >
>
> Thank you. I'd be very motivated to place them in the quantum framework
> structure. To begin with, we didn't want to propose directly and wanted to get
> comments if such tests could be useful (which seems like they are). Also,
> currently, at the quantum layer we dont have a mysql database that these tests
> need. I'm thinking of using in-memory database for these tests to run at the
> quantum level. I also have some extra tests within this class that needs
> changes at the db/api module (mentioned below). If you think the way it is
> currently is ok, then I already have this as an action item to followup/work
> on this as a separate activity.
>

I think they can stay in the cisco directory at the moment. I think we should move them into the main "tests" folder before diablo release though. I will post to the mailing list in order to decide together the best course of action.

Rohit: Sounds like a plan.

>
> > 3517 - 3905: L2NetworkDB and QuantumDB are wrappers around database apis
> uses
> > by test cases. For improve code readability, these classes are typycally put
> > in a separate module. (see /tests/unit/extensions)
> >
>
> Absolutely, I'd make this activity as part of moving the reusable tests to the
> quantum level (mentioned in the above comment)
>

No problem.

Rohit: Thanks.

> > Curiosities:
> >
> > 807 === added file 'quantum/plugins/cisco/db/api.py'
> >
> > Have you considered re-using the code in quantum/db/api.py? If yes, why
> wasn't
> > this code suitable for your needs? I'm asking because it seems this module
> > seems very similar to the db api in quantum.
> >
> > Similar comment for the module quantum/plugins/cisco/db/models.py
> > If the reason is the InnoDB engine then we can try and improve modules in
> > quantum/db, thus avoiding code duplication.
> >
>
> Yes, we have indeed considered to reuse this code. We have also preserved the
> credits/authors so that the reviewers are aware of it. Like its mentioned in
> the merge proposal, we made couple of changes in these modules -
> * specify mysql engine for models (InnoDB) - this engine imposes foreign key
> constraints
> * early loading of tables using joinedload in queries - this allows to
> exercise the relation
> attribute defined between network & port tables
> Our motivation again, was to get views on these changes and if they look good
> incorporate them at the quantum layer (as a seperate activity as it aligns
> with the earlier responses).
>

We can leave things this way at the moment, and then discuss about how to improve them at the design summit.

Rohit: Sounds good.

> > 1489 +class VlanID(BASE, L2NetworkBase):
> >
> > Have you considered adding constraint to ensure only valid VLAN IDs are
> > entered into the database?
> >
>
> We create vlanids in this table at the start using the specified vlan start
> and end range in the conf file. Any changes in the configuration after that is
> not currently handled.
>

I was actuall referring to the fact that a user can enter negative values or values > 4096 for start and end VLAN IDs. In this case these invalid values could end up in the db. If this turns out to be an issue, we can file a bug for it.

Rohit: Ah, I understand your suggestion now and it's a good point.

All my comments have been addressed, either in the code, or with reasonable arguments by Rohit and Edgar. I'm therefore approving the merge proposal.

Rohit: Thanks Brad and Salvatore for the review comments and merging the branch to lp:quantum.