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

Hi Rohit,

thanks a lot for proposing this significant improvement on the Cisco plugin.
The code looks good, and, as usual, it is very well written and easy to understand.

I have a few comments below aimed at improving it or understanding whether some bits of it can be "promoted" to quantum core. Please let me know if you have any question.

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

1104 +def create_vlanids():
1105 + """Prepopulates the vlan_bindings table"""
1106 + session = db.get_session()
1107 + try:
1108 + vlanid = session.query(l2network_models.VlanID).\
1109 + one()
1110 + except exc.MultipleResultsFound:
1111 + pass
1112 + except exc.NoResultFound:
1113 + start = int(conf.VLAN_START)
1114 + end = int(conf.VLAN_END)
1115 + while start <= end:
1116 + vlanid = l2network_models.VlanID(start)
1117 + session.add(vlanid)
1118 + start += 1
1119 + session.flush()
1120 + return

IMHO this routine could be improved. Pre-population is skipped if a single record is found in the DB. This means that if I change VLAN_START or VLAN_END and I do not destroy the DB, the DB will not be updated.

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

Minor comments:

335 + 5b. Enter the quantum_l2netowrk database configuration info in the
336 + quantum/plugins/cisco/conf/db_conn.ini file.

There's a typo in the first line (should be quantum_l2network)

348 your configuration of each of the above files hasn't gone a little kaka

'kaka'? Are you referring to the Real Madrid's Brazilian player :) ? I guess it means something like "screwed". No need to change it, it is just that this term is totally new to me.

1093 +import l2network_models

Please avoid relative imports

1180 + vlanids = session.query(l2network_models.VlanID).\
1181 + filter_by(vlan_used=False).\
1182 + all()
1183 + rvlan = vlanids[0]

IMHO using first() instead of all() will improve the efficiency of this routine.

1747 + LOG.debug("Loaded device plugin %s\n" % \

Python logging already adds newline at the end of line. This newline is therefore redundant, unless we need it for other reasons.

2788 -2789 : Indentation can be improved here

2798 + confstr = snipp.CMD_VLAN_CONF_SNIPPET % (vlanid, vlanname)
2799 + confstr = snipp.EXEC_CONF_PREFIX + confstr + snipp.EXEC_CONF_POSTFIX

Consider merging EXEC_CONF_PREFIX and POSTIFIX into a EXEC_CONF_SNIPPET.
Also, since line 2799 is repeated several times, consider moving it into a subroutine.

3144 +FILTER_SHOW_VLAN_BRIEF_SNIPPET = """
3145 + <show xmlns="http://www.cisco.com/nxos:1.0:vlan_mgr_cli">
3146 + <vlan>
3147 + <brief/>
3148 + </vlan>
3149 + </show> """

End delimiter for a docstring is usually in a new line, as you did for other docstrings in the same module.

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)

4332; 4338; 4359; 4440-4444; 4566-4571; 4584; 4586-4587; 4655-4662: Please remove commented code lines. (also found a couple of times in test_ucs_plugin.py)

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.

1489 +class VlanID(BASE, L2NetworkBase):

Have you considered adding constraint to ensure only valid VLAN IDs are entered into the database?

2325 + # TODO (Sumit): Need to check the following
2326 + port_id = None
2327 + cdb.add_pp_binding(tenant_id, port_id, portprofile[const.UUID], True)

I see there's already a TODO comment on these lines. Just wondering why we would need to create a port profile binding to a non-existent port.