OpenStack Compute (nova)

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

  1. empty-network-lp835242
  2. Merge into trunk
Proposed by Brian Lamar
Status: Merged
Approved by: Devin Carlen
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
Rick Harris (community) Approve
Vish Ishaya (community) Approve
Brian Waldon (community) Approve
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.
lp:~rackspace-titan/nova/empty-network-lp835242 updated
1525. By Brian Lamar

Probably shouldn't leave that commented out.

Revision history for this message
Brian Waldon (bcwaldon) wrote :

Looks good.

review: Approve
Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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
Revision history for this message
Rick Harris (rconradharris) wrote :

LGTM.

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

review: Approve
Revision history for this message
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?

lp:~rackspace-titan/nova/empty-network-lp835242 updated
1526. By Brian Lamar

Merged trunk.

Revision history for this message
Devin Carlen (devcamcar) wrote :

agreed! let's get this in.

review: Approve
Revision history for this message
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
=== modified file 'nova/api/openstack/views/addresses.py'
--- nova/api/openstack/views/addresses.py 2011-08-23 03:30:12 +0000
+++ nova/api/openstack/views/addresses.py 2011-09-07 18:21:40 +0000
@@ -88,7 +88,6 @@
88 try:88 try:
89 return interface['network']['label']89 return interface['network']['label']
90 except (TypeError, KeyError) as exc:90 except (TypeError, KeyError) as exc:
91 LOG.exception(exc)
92 raise TypeError91 raise TypeError
9392
94 def _extract_ipv4_addresses(self, interface):93 def _extract_ipv4_addresses(self, interface):
9594
=== modified file 'nova/network/manager.py'
--- nova/network/manager.py 2011-09-07 17:46:55 +0000
+++ nova/network/manager.py 2011-09-07 18:21:40 +0000
@@ -484,6 +484,9 @@
484 for vif in vifs:484 for vif in vifs:
485 network = vif['network']485 network = vif['network']
486486
487 if network is None:
488 continue
489
487 # determine which of the instance's IPs belong to this network490 # determine which of the instance's IPs belong to this network
488 network_IPs = [fixed_ip['address'] for fixed_ip in fixed_ips if491 network_IPs = [fixed_ip['address'] for fixed_ip in fixed_ips if
489 fixed_ip['network_id'] == network['id']]492 fixed_ip['network_id'] == network['id']]
490493
=== modified file 'nova/tests/test_network.py'
--- nova/tests/test_network.py 2011-08-30 07:03:39 +0000
+++ nova/tests/test_network.py 2011-09-07 18:21:40 +0000
@@ -118,9 +118,14 @@
118 {'id': 1,118 {'id': 1,
119 'address': 'DE:AD:BE:EF:00:01',119 'address': 'DE:AD:BE:EF:00:01',
120 'uuid': '00000000-0000-0000-0000-0000000000000001',120 'uuid': '00000000-0000-0000-0000-0000000000000001',
121 'network_id': 0,
122 'network_id': 1,121 'network_id': 1,
123 'network': FakeModel(**networks[1]),122 'network': FakeModel(**networks[1]),
123 'instance_id': 0},
124 {'id': 2,
125 'address': 'DE:AD:BE:EF:00:02',
126 'uuid': '00000000-0000-0000-0000-0000000000000002',
127 'network_id': 2,
128 'network': None,
124 'instance_id': 0}]129 'instance_id': 0}]
125130
126131