Merge lp:~rackspace-titan/nova/empty-network-lp835242 into lp:~hudson-openstack/nova/trunk

Proposed by Brian Lamar on 2011-09-01
Status: Merged
Approved by: Devin Carlen on 2011-09-07
Approved revision: 1526
Merged at revision: 1531
Proposed branch: lp:~rackspace-titan/nova/empty-network-lp835242
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 43 lines (+9/-2)
3 files modified
nova/api/openstack/views/addresses.py (+0/-1)
nova/network/manager.py (+3/-0)
nova/tests/test_network.py (+6/-1)
To merge this branch: bzr merge lp:~rackspace-titan/nova/empty-network-lp835242
Reviewer Review Type Date Requested Status
Devin Carlen (community) Approve on 2011-09-07
Rick Harris (community) Approve on 2011-09-02
Vish Ishaya (community) 2011-09-01 Approve on 2011-09-01
Brian Waldon (community) 2011-09-01 Approve on 2011-09-01
Review via email: mp+73716@code.launchpad.net

Description of the Change

Fixes a case where if a VIF is returned with a NULL network it might not be able to be deleted. Added test case for that fix.

To post a comment you must log in.
1525. By Brian Lamar on 2011-09-01

Probably shouldn't leave that commented out.

Brian Waldon (bcwaldon) wrote :

Looks good.

review: Approve
Vish Ishaya (vishvananda) wrote :

LGTM. Curiosity:

7 except (TypeError, KeyError) as exc:
8 - LOG.exception(exc)
9 raise TypeError

Why are we catching and reraising a different exception?

review: Approve
Brian Waldon (bcwaldon) wrote :

> LGTM. Curiosity:
>
> 7 except (TypeError, KeyError) as exc:
> 8 - LOG.exception(exc)
> 9 raise TypeError
>
> Why are we catching and reraising a different exception?

Since I wrote it, I'll answer: I didn't want to create a new exception, and it didn't matter if it was a KeyError or a TypeError in the calling code. I will admit it's kind of fragile.

Brian Lamar (blamar) wrote :

I saw that as well...I started to update the logic but it *works* as intended, so I didn't.

Really instead of doing logic with exception handling we should be returning None and then handling that case in the methods that call it. It should be updated at some point to make it less confusing but it works for the time being.

Devin Carlen (devcamcar) wrote :

Can we add a bug for fixing the exception handling? Then I think we are clear to merge this.

review: Needs Information
Rick Harris (rconradharris) wrote :

LGTM.

++ on adding a bug to fix the exception handling here.

review: Approve
Brian Lamar (blamar) wrote :

> Can we add a bug for fixing the exception handling? Then I think we are clear
> to merge this.

I'm all for re-working this method, but there is no bug here as far as I'm concerned. This was all about removing a logging call which made it look like an error was occurring and checking to see if network was None.

I don't see any other bugs, so I'm a bit confused why it would hold up this prop.

I have this: http://bazaar.launchpad.net/~rackspace-titan/nova/exception-flow-logic/revision/1527

But I'm still not convinced that the way Waldon did it initially isn't actually more robust. Can we at least agree that I'm looking into making it better and get this fix in?

1526. By Brian Lamar on 2011-09-07

Merged trunk.

Devin Carlen (devcamcar) wrote :

agreed! let's get this in.

review: Approve
Trey Morris (tr3buchet) wrote :

wish i'd caught this earlier... This bug can't happen in trunk. It was a private branch issue. I'll just revert parts of it in a separate branch. :(

sorry

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/openstack/views/addresses.py'
2--- nova/api/openstack/views/addresses.py 2011-08-23 03:30:12 +0000
3+++ nova/api/openstack/views/addresses.py 2011-09-07 18:21:40 +0000
4@@ -88,7 +88,6 @@
5 try:
6 return interface['network']['label']
7 except (TypeError, KeyError) as exc:
8- LOG.exception(exc)
9 raise TypeError
10
11 def _extract_ipv4_addresses(self, interface):
12
13=== modified file 'nova/network/manager.py'
14--- nova/network/manager.py 2011-09-07 17:46:55 +0000
15+++ nova/network/manager.py 2011-09-07 18:21:40 +0000
16@@ -484,6 +484,9 @@
17 for vif in vifs:
18 network = vif['network']
19
20+ if network is None:
21+ continue
22+
23 # determine which of the instance's IPs belong to this network
24 network_IPs = [fixed_ip['address'] for fixed_ip in fixed_ips if
25 fixed_ip['network_id'] == network['id']]
26
27=== modified file 'nova/tests/test_network.py'
28--- nova/tests/test_network.py 2011-08-30 07:03:39 +0000
29+++ nova/tests/test_network.py 2011-09-07 18:21:40 +0000
30@@ -118,9 +118,14 @@
31 {'id': 1,
32 'address': 'DE:AD:BE:EF:00:01',
33 'uuid': '00000000-0000-0000-0000-0000000000000001',
34- 'network_id': 0,
35 'network_id': 1,
36 'network': FakeModel(**networks[1]),
37+ 'instance_id': 0},
38+ {'id': 2,
39+ 'address': 'DE:AD:BE:EF:00:02',
40+ 'uuid': '00000000-0000-0000-0000-0000000000000002',
41+ 'network_id': 2,
42+ 'network': None,
43 'instance_id': 0}]
44
45