From 61fc1b9ee11e416aecbf3a29e1d150a53fc890e8 Mon Sep 17 00:00:00 2001 From: Andrew Laski Date: Wed, 9 Mar 2016 14:04:36 -0500 Subject: [PATCH] Change SpawnIsSynchronous fixture return In normal usage utils.spawn returns an eventlet.greenthread.GreenThread object which has a few methods defined on it. Currently the wait() and link() methods are used in Nova. However the SpawnIsSynchronous fixture was returning the results of wait() directly which led to an interface mismatch. Normally utils.spawn_n returns a greenlet object which does not allow checking the return but in testing we have never blocked that capability before. This could be enhanced later to be strict for spawn_n if we choose, but it is handy to have raised exceptions pass through in testing. SpawnIsSynchronous now returns a GreenThread object lookalike which has wait() and link() methods that work as expected. This allows for the removal of the wait() method from network/model.py:NetworkInfo. A few mocks which emulated the improper behavior were also updated. Change-Id: I1ded81f93235e217822e385ddd983b24c6359a53 --- nova/network/model.py | 11 ------ nova/tests/fixtures.py | 30 ++++++++++++++-- nova/tests/unit/compute/test_compute_mgr.py | 10 +++--- nova/tests/unit/test_fixtures.py | 40 +++++++++++++++++++++ 4 files changed, 73 insertions(+), 18 deletions(-) diff --git a/nova/network/model.py b/nova/network/model.py index c6a089d87f2..b28bc37aa9c 100644 --- a/nova/network/model.py +++ b/nova/network/model.py @@ -464,17 +464,6 @@ def hydrate(cls, network_info): def json(self): return jsonutils.dumps(self) - def wait(self, do_raise=True): - """Return this NetworkInfo - - This is useful to avoid type checking when it's not clear if you have a - NetworkInfo or NetworkInfoAsyncWrapper. It's also useful when - utils.spawn in NetworkInfoAsyncWrapper is patched to return a - NetworkInfo rather than a GreenThread and wait() should return the same - thing for both cases. - """ - return self - class NetworkInfoAsyncWrapper(NetworkInfo): """Wrapper around NetworkInfo that allows retrieving NetworkInfo diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index 491f2f4c792..849a9ad3ea4 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -460,15 +460,41 @@ def setUp(self): self.addCleanup(self.cleanup) +class _FakeGreenThread(object): + def __init__(self, func, *args, **kwargs): + self._result = func(*args, **kwargs) + + def cancel(self, *args, **kwargs): + # This method doesn't make sense for a synchronous call, it's just + # defined to satisfy the interface. + pass + + def kill(self, *args, **kwargs): + # This method doesn't make sense for a synchronous call, it's just + # defined to satisfy the interface. + pass + + def link(self, func, *args, **kwargs): + func(self, *args, **kwargs) + + def unlink(self, func, *args, **kwargs): + # This method doesn't make sense for a synchronous call, it's just + # defined to satisfy the interface. + pass + + def wait(self): + return self._result + + class SpawnIsSynchronousFixture(fixtures.Fixture): """Patch and restore the spawn_n utility method to be synchronous""" def setUp(self): super(SpawnIsSynchronousFixture, self).setUp() self.useFixture(fixtures.MonkeyPatch( - 'nova.utils.spawn_n', lambda f, *a, **k: f(*a, **k))) + 'nova.utils.spawn_n', _FakeGreenThread)) self.useFixture(fixtures.MonkeyPatch( - 'nova.utils.spawn', lambda f, *a, **k: f(*a, **k))) + 'nova.utils.spawn', _FakeGreenThread)) class BannedDBSchemaOperations(fixtures.Fixture): diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index d57a039d8c7..215e6cb628b 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -3068,6 +3068,8 @@ def setUp(self): 'hosts': [[self.compute.host, 'fake-node']]}} + self.useFixture(fixtures.SpawnIsSynchronousFixture()) + def fake_network_info(): return network_model.NetworkInfo([{'address': '1.2.3.4'}]) @@ -3081,8 +3083,6 @@ def fake_network_info(): self.compute.driver, self.node) self.compute._resource_tracker_dict[self.node] = fake_rt - self.useFixture(fixtures.SpawnIsSynchronousFixture()) - def _do_build_instance_update(self, reschedule_update=False): self.mox.StubOutWithMock(self.instance, 'save') self.instance.save( @@ -3652,7 +3652,7 @@ def _test_build_and_run_spawn_exceptions(self, exc): side_effect=[self.instance, self.instance, self.instance]), mock.patch.object(self.compute, '_build_networks_for_instance', - return_value=network_model.NetworkInfo()), + return_value=self.network_info), mock.patch.object(self.compute, '_notify_about_instance_usage'), mock.patch.object(self.compute, @@ -3696,7 +3696,7 @@ def _test_build_and_run_spawn_exceptions(self, exc): _shutdown_instance.assert_called_once_with(self.context, self.instance, self.block_device_mapping, - self.requested_networks, try_deallocate_networks=True) + self.requested_networks, try_deallocate_networks=False) def test_reschedule_on_resources_unavailable(self): reason = 'resource unavailable' @@ -3851,7 +3851,7 @@ def test_build_resources_with_network_info_obj_on_spawn_failure(self): self.mox.StubOutWithMock(self.compute, '_shutdown_instance') self.compute._build_networks_for_instance(self.context, self.instance, self.requested_networks, self.security_groups).AndReturn( - network_model.NetworkInfo([{'address': '1.2.3.4'}])) + self.network_info) self.compute._shutdown_instance(self.context, self.instance, self.block_device_mapping, self.requested_networks, try_deallocate_networks=False) diff --git a/nova/tests/unit/test_fixtures.py b/nova/tests/unit/test_fixtures.py index 9e9e1980af9..be170d97ac1 100644 --- a/nova/tests/unit/test_fixtures.py +++ b/nova/tests/unit/test_fixtures.py @@ -342,6 +342,46 @@ def test_spawn_passes_through(self): utils.spawn_n(tester.function, 'foo', bar='bar') tester.function.assert_called_once_with('foo', bar='bar') + def test_spawn_return_has_wait(self): + self.useFixture(fixtures.SpawnIsSynchronousFixture()) + gt = utils.spawn(lambda x: '%s' % x, 'foo') + foo = gt.wait() + self.assertEqual('foo', foo) + + def test_spawn_n_return_has_wait(self): + self.useFixture(fixtures.SpawnIsSynchronousFixture()) + gt = utils.spawn_n(lambda x: '%s' % x, 'foo') + foo = gt.wait() + self.assertEqual('foo', foo) + + def test_spawn_has_link(self): + self.useFixture(fixtures.SpawnIsSynchronousFixture()) + gt = utils.spawn(mock.MagicMock) + passed_arg = 'test' + call_count = [] + + def fake(thread, param): + self.assertEqual(gt, thread) + self.assertEqual(passed_arg, param) + call_count.append(1) + + gt.link(fake, passed_arg) + self.assertEqual(1, len(call_count)) + + def test_spawn_n_has_link(self): + self.useFixture(fixtures.SpawnIsSynchronousFixture()) + gt = utils.spawn_n(mock.MagicMock) + passed_arg = 'test' + call_count = [] + + def fake(thread, param): + self.assertEqual(gt, thread) + self.assertEqual(passed_arg, param) + call_count.append(1) + + gt.link(fake, passed_arg) + self.assertEqual(1, len(call_count)) + class TestBannedDBSchemaOperations(testtools.TestCase): def test_column(self):