Merge lp:~tamura-yoshiaki/nova/bug-764236 into lp:~hudson-openstack/nova/trunk

Proposed by Yoshiaki Tamura
Status: Merged
Approved by: Vish Ishaya
Approved revision: 996
Merged at revision: 1030
Proposed branch: lp:~tamura-yoshiaki/nova/bug-764236
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 60 lines (+38/-1)
2 files modified
nova/tests/test_virt.py (+37/-0)
nova/virt/libvirt_conn.py (+1/-1)
To merge this branch: bzr merge lp:~tamura-yoshiaki/nova/bug-764236
Reviewer Review Type Date Requested Status
Vish Ishaya (community) Approve
Devin Carlen (community) Approve
termie (community) Approve
Review via email: mp+58068@code.launchpad.net

Commit message

Add a test checking spawn() works when network_info is set, which
currently doesn't.
The following patch would fix parameter mismatch calling _create_image() from spawn() in
libvirt_conn.py

To post a comment you must log in.
Revision history for this message
Jesse Andrews (anotherjesse) wrote :

Yoshiaki - is there a way to add a smoketest or unit test that fails without your fix?

Revision history for this message
Yoshiaki Tamura (tamura-yoshiaki) wrote :

On Mon, Apr 18, 2011 at 2:20 PM, anotherjesse <email address hidden> wrote:
> Yoshiaki - is there a way to add a smoketest or unit test that fails without your fix?

Although I'm quite new to these testing, I think it should be possible
if you call spawn() with network_info set.

> --
> https://code.launchpad.net/~tamura-yoshiaki/nova/bug-764236/+merge/58068
> You are the owner of lp:~tamura-yoshiaki/nova/bug-764236.
>

Revision history for this message
Jesse Andrews (anotherjesse) wrote :

A unit test would be adding a test to nova/tests/test_compute.py that resulted in network_info being set and causing exceptions to occur until your patch is applied.

Sometimes errors only really manifest in deployed systems (for which we have smoketests - a series of tests that run against the API of a deployed system).

How did you happen upon the bug in the first place - that might help with determining how we can add a test :)

Revision history for this message
Yoshiaki Tamura (tamura-yoshiaki) wrote :

On Mon, Apr 18, 2011 at 4:24 PM, anotherjesse <email address hidden> wrote:
> A unit test would be adding a test to nova/tests/test_compute.py that resulted in network_info being set and causing exceptions to occur until your patch is applied.
>
> Sometimes errors only really manifest in deployed systems (for which we have smoketests - a series of tests that run against the API of a deployed system).
>
> How did you happen upon the bug in the first place - that might help with determining how we can add a test :)

Thanks, Jesse. I would try making a simple test case although it may
be difficult because I'm just coming across with these issues through
static review :)

Yoshi

> --
> https://code.launchpad.net/~tamura-yoshiaki/nova/bug-764236/+merge/58068
> You are the owner of lp:~tamura-yoshiaki/nova/bug-764236.
>

Revision history for this message
Yoshiaki Tamura (tamura-yoshiaki) wrote :

Hi Jesse,

On Mon, Apr 18, 2011 at 4:24 PM, anotherjesse <email address hidden> wrote:
> A unit test would be adding a test to nova/tests/test_compute.py that resulted in network_info being set and causing exceptions to occur until your patch is applied.
>
> Sometimes errors only really manifest in deployed systems (for which we have smoketests - a series of tests that run against the API of a deployed system).
>
> How did you happen upon the bug in the first place - that might help with determining how we can add a test :)

Although it may not be a good test, I've attached a test which fails
without my patch. Could you take a look?
Without my patch, the exception that happens beneath would be TypeError.

Thanks,

Yoshi

=== modified file 'nova/tests/test_virt.py'
--- nova/tests/test_virt.py 2011-04-07 21:48:29 +0000
+++ nova/tests/test_virt.py 2011-04-19 08:42:23 +0000
@@ -549,6 +549,44 @@
         db.volume_destroy(self.context, volume_ref['id'])
         db.instance_destroy(self.context, instance_ref['id'])

+ def test_spawn_with_network_info(self):
+ # Skip if non-libvirt environment
+ if not self.lazy_load_library_exists():
+ return
+
+ # Preparing mocks
+ def fake_none(self, instance):
+ return
+
+ self.create_fake_libvirt_mock()
+ instance = db.instance_create(self.context, self.test_instance)
+
+ # Start test
+ self.mox.ReplayAll()
+ conn = libvirt_conn.LibvirtConnection(False)
+ conn.firewall_driver.setattr('setup_basic_filtering', fake_none)
+ conn.firewall_driver.setattr('prepare_instance_filter', fake_none)
+
+ network = db.project_get_network(context.get_admin_context(),
+ self.project.id)
+ ip_dict = {'ip': self.test_ip,
+ 'netmask': network['netmask'],
+ 'enabled': '1'}
+ mapping = {
+ 'label': network['label'],
+ 'gateway': network['gateway'],
+ 'mac': instance['mac_address'],
+ 'dns': [network['dns']],
+ 'ips': [ip_dict]}
+ network_info = [(network, mapping)]
+
+ try:
+ conn.spawn(instance, network_info)
+ except Exception, e:
+ count = (0 <= e.message.find('Unexpected method call'))
+
+ self.assertTrue(count)
+
     def tearDown(self):
         self.manager.delete_project(self.project)
         self.manager.delete_user(self.user)

Revision history for this message
termie (termie) wrote :

Looks like you've got a couple pep8 errors in there still, you may want to use run_tests.sh again or grab this helper (i have a merge prop out to add it ./tools/pep8.sh): http://term.ie/data/pep8.sh

Also, pep8 doesn't appear to catch the extra whitespaces on the first line of ip_dict (diff line 28), between the key and the value.

Could you move the contents of the mapping dict up a line so that they begin on the same line as the mapping declaration (like ip_dict above it)?

Your test looks like it verifies your code, so works for me :)

review: Needs Fixing
Revision history for this message
Yoshiaki Tamura (tamura-yoshiaki) wrote :

On Thu, Apr 21, 2011 at 3:32 AM, termie <email address hidden> wrote:
> Review: Needs Fixing
> Looks like you've got a couple pep8 errors in there still, you may want to use run_tests.sh again or grab this helper (i have a merge prop out to add it ./tools/pep8.sh): http://term.ie/data/pep8.sh
>
> Also, pep8 doesn't appear to catch the extra whitespaces on the first line of ip_dict (diff line 28), between the key and the value.
>
> Could you move the contents of the mapping dict up a line so that they begin on the same line as the mapping declaration (like ip_dict above it)?

Sorry about that termie. I checked with pep8, and also cleaned up
trailing whitespaces.
I hope everything is fine this time.

Yoshi

>
> Your test looks like it verifies your code, so works for me :)
>
>
> --
> https://code.launchpad.net/~tamura-yoshiaki/nova/bug-764236/+merge/58068
> You are the owner of lp:~tamura-yoshiaki/nova/bug-764236.
>

Revision history for this message
termie (termie) wrote :

lgtm

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

lgtm

review: Approve
Revision history for this message
Vish Ishaya (vishvananda) wrote :

lgtm

review: Approve
Revision history for this message
Soren Hansen (soren) wrote :
Revision history for this message
Vish Ishaya (vishvananda) wrote :

Sigh, I usually notice this and write a commit message when I approve. Sorry for spamming irc with jenkins notices. It would be great if we changed jenkins to actually switch this back to Needs Review instead of just failing over and over.

Yoshiaki Tamura, Can you please put in a commit messasge or description so we can merge this?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'nova/tests/test_virt.py'
--- nova/tests/test_virt.py 2011-04-07 21:48:29 +0000
+++ nova/tests/test_virt.py 2011-04-21 08:01:09 +0000
@@ -549,6 +549,43 @@
549 db.volume_destroy(self.context, volume_ref['id'])549 db.volume_destroy(self.context, volume_ref['id'])
550 db.instance_destroy(self.context, instance_ref['id'])550 db.instance_destroy(self.context, instance_ref['id'])
551551
552 def test_spawn_with_network_info(self):
553 # Skip if non-libvirt environment
554 if not self.lazy_load_library_exists():
555 return
556
557 # Preparing mocks
558 def fake_none(self, instance):
559 return
560
561 self.create_fake_libvirt_mock()
562 instance = db.instance_create(self.context, self.test_instance)
563
564 # Start test
565 self.mox.ReplayAll()
566 conn = libvirt_conn.LibvirtConnection(False)
567 conn.firewall_driver.setattr('setup_basic_filtering', fake_none)
568 conn.firewall_driver.setattr('prepare_instance_filter', fake_none)
569
570 network = db.project_get_network(context.get_admin_context(),
571 self.project.id)
572 ip_dict = {'ip': self.test_ip,
573 'netmask': network['netmask'],
574 'enabled': '1'}
575 mapping = {'label': network['label'],
576 'gateway': network['gateway'],
577 'mac': instance['mac_address'],
578 'dns': [network['dns']],
579 'ips': [ip_dict]}
580 network_info = [(network, mapping)]
581
582 try:
583 conn.spawn(instance, network_info)
584 except Exception, e:
585 count = (0 <= e.message.find('Unexpected method call'))
586
587 self.assertTrue(count)
588
552 def tearDown(self):589 def tearDown(self):
553 self.manager.delete_project(self.project)590 self.manager.delete_project(self.project)
554 self.manager.delete_user(self.user)591 self.manager.delete_user(self.user)
555592
=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py 2011-04-18 23:40:03 +0000
+++ nova/virt/libvirt_conn.py 2011-04-21 08:01:09 +0000
@@ -616,7 +616,7 @@
616 'launching')616 'launching')
617 self.firewall_driver.setup_basic_filtering(instance, network_info)617 self.firewall_driver.setup_basic_filtering(instance, network_info)
618 self.firewall_driver.prepare_instance_filter(instance, network_info)618 self.firewall_driver.prepare_instance_filter(instance, network_info)
619 self._create_image(instance, xml, network_info)619 self._create_image(instance, xml, network_info=network_info)
620 domain = self._create_new_domain(xml)620 domain = self._create_new_domain(xml)
621 LOG.debug(_("instance %s: is running"), instance['name'])621 LOG.debug(_("instance %s: is running"), instance['name'])
622 self.firewall_driver.apply_instance_filter(instance)622 self.firewall_driver.apply_instance_filter(instance)