Merge lp://staging/~tylesmit/neutron/lp845140 into lp://staging/neutron/diablo

Hi Tyler, I've done a quick scan of the code and nothing stood out as being problematic. I didn't read the code for correctness though.

Given that this is very late in the cycle and a pretty large patch, it may be challenging to get this in. If you can get another reviewer, I'll count my quick review as an approve and merge it in, since it is entirely contained within the Cisco plugin.

Hi Tyler.
As already stated by Dan this mergeprop came a bit too close to the deadline. I have a few comments for you and I hope you can address them before rbp is released.

None of the issues I found appear to break the plugin, but my judgement is merely based on static code analysis. Moreover, I would not consider this review that I've done as 'accurate'; however since the risk is 'contained' to the Cisco plugin, I'm willing to approve it.

29 for device_ip in device_ips:
30 new_device_params = deepcopy(device_params)
Looks like there no need to perfom the deepcopy operation in the loop.

31 new_device_params[const.DEVICE_IP] = device_ip
At a first glance this seems a workaround needed as _invoke_plugin with the specific function used in this case does not accept a list of IPs. Probably the ideal approach would be to update _invoke_plugin to deal with multiple ips. But this is not a problem at all.

226 + """Tear down our tests"""
227 + try:
228 + self._l2network_multiblade.delete_port([tenant_id, self.net_id,
229 + self.port_id])
230 + except exc.PortNotFound:
231 + # We won't always have a port to remove
232 + pass
233 +
234 + try:
235 + self._l2network_multiblade.delete_network([tenant_id, self.net_id])
236 + except exc.NetworkNotFound:
237 + # We won't always have a network to remove
238 + pass

The above is probably not necessary as the last line in tearDown destroys all the contents of the test database.

538 +class TestUCSInventory(unittest.TestCase):

This class has not teardown method for clearing the database.

1544 + def tearDown(self):
1545 + """Clear the test environment"""
1546 + # Remove database contents
1547 + db.clear_db()

Not very useful, but probably still good to have, as this class has methods for tearing down reasources created in each unit test.

Hi Salvatore,

>Looks like there no need to perfom the deepcopy operation in the loop.

This is not code I'm adding, it's merely shown in the diff because of its proximity to code I did alter.

>At a first glance this seems a workaround needed as _invoke_plugin with the specific function used in this case does not accept a list of IPs. Probably the ideal approach would be to update _invoke_plugin to deal with multiple ips. But this is not a problem at all.

Same as above.

>The above is probably not necessary as the last line in tearDown destroys all the contents of the test database.

These calls aren't doing anything to the database. They're cleaning up the created networks and ports on the UCS devices themselves.

>This class has not teardown method for clearing the database.

Good call. I'll add that.

>Not very useful, but probably still good to have, as this class has methods for tearing down reasources created in each unit test.

I don't know much about how sqlite in-memory works. I figured it would force sqlite to free the used memory, otherwise it may stick around. That could be completely wrong though.

Hi Tyler,

I'm looking to create the diablo-rbp release soon (prior to 6pm Pacific
today). If you're planning on making these changes quickly, let me know and
I'll hold things up. Otherwise, we can put them in after the release,
though I'd prefer to avoid that.

dan

On Fri, Sep 9, 2011 at 3:42 PM, Tyler Smith <email address hidden> wrote:

> Hi Salvatore,
>
> >Looks like there no need to perfom the deepcopy operation in the loop.
>
> This is not code I'm adding, it's merely shown in the diff because of its
> proximity to code I did alter.
>
> >At a first glance this seems a workaround needed as _invoke_plugin with
> the specific function used in this case does not accept a list of IPs.
> Probably the ideal approach would be to update _invoke_plugin to deal with
> multiple ips. But this is not a problem at all.
>
> Same as above.
>
> >The above is probably not necessary as the last line in tearDown destroys
> all the contents of the test database.
>
> These calls aren't doing anything to the database. They're cleaning up the
> created networks and ports on the UCS devices themselves.
>
>
> >This class has not teardown method for clearing the database.
>
> Good call. I'll add that.
>
>
> >Not very useful, but probably still good to have, as this class has
> methods for tearing down reasources created in each unit test.
>
> I don't know much about how sqlite in-memory works. I figured it would
> force sqlite to free the used memory, otherwise it may stick around. That
> could be completely wrong though.
> --
> https://code.launchpad.net/~tylesmit/quantum/lp845140/+merge/74826
> You are reviewing the proposed merge of lp:~tylesmit/quantum/lp845140 into
> lp:quantum.
>

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~

Suggest checking whether the corresponding inventory class is configured for a plugin before doing conf.PLUGINS[const.INVENTORY][key]

219 + self._inventory[key] = utils.import_object(
220 + conf.PLUGINS[const.INVENTORY][key])

Something like:

            if key in conf.PLUGINS[const.INVENTORY].keys():
                self._inventory[key] = utils.import_object(
                    conf.PLUGINS[const.INVENTORY][key])

should work.

Can you please change the following to reflect the test module:

190 + """
191 + Implements the L2NetworkModelBase
192 + This implementation works with UCS and Nexus plugin for the
193 + following topology:
194 + One or more UCSM (each with one or more chasses connected)
195 + All UCSM connected to a single Nexus Switch
196 + """

Thanks for the reviews. I've got all of the issues resolved, and will have them pushed shortly.

Okay, I've pushed fixes for all of the discusses issues. Sorry for taking it to the 11th hour (or 11th:45), and thanks again for the last minute reviews.

It should be ready to merge.

Dan, I believe Tyler has made all the changes, and this branch is ready to be merged.

(He is trying to respond but having issues posting here.)

Thanks,
~Sumit.

Ok, thanks. I will merge it in and close diablo-rbp.

Dan

On Fri, Sep 9, 2011 at 4:25 PM, Sumit Naiksatam <email address hidden> wrote:

> Dan, I believe Tyler has made all the changes, and this branch is ready to
> be merged.
>
> (He is trying to respond but having issues posting here.)
>
> Thanks,
> ~Sumit.
> --
> https://code.launchpad.net/~tylesmit/quantum/lp845140/+merge/74826
> You are reviewing the proposed merge of lp:~tylesmit/quantum/lp845140 into
> lp:quantum.
>

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~