From 37bc3e1ca641e70eefa902d8f366a4369bd509ad Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Wed, 10 Jun 2015 11:40:28 +0100 Subject: [PATCH] Change return value of [driver_]vendor_passthru to dict This is a refactor patch that changes the return value of the vendor_passthru() and driver_vendor_passthru() RPC methods to a dictionary instead of a tuple. By doing that we make it easy to extend for future improvements. The RPC version was bumped to 1.29 by this change. Change-Id: I8729bb85d97a563d4ec8409ecd0eba15dc0c51bc Related-Blueprint: ipxe-dynamic-config --- ironic/api/controllers/v1/driver.py | 6 ++--- ironic/api/controllers/v1/node.py | 6 ++--- ironic/conductor/manager.py | 32 +++++++++++++++----------- ironic/conductor/rpcapi.py | 26 ++++++++++++--------- ironic/tests/api/v1/test_drivers.py | 25 ++++++++++---------- ironic/tests/api/v1/test_nodes.py | 16 ++++++------- ironic/tests/conductor/test_manager.py | 32 +++++++++++++------------- 7 files changed, 77 insertions(+), 66 deletions(-) diff --git a/ironic/api/controllers/v1/driver.py b/ironic/api/controllers/v1/driver.py index 582670ffca..465ccfa21f 100644 --- a/ironic/api/controllers/v1/driver.py +++ b/ironic/api/controllers/v1/driver.py @@ -150,11 +150,11 @@ class DriverPassthruController(rest.RestController): http_method = pecan.request.method.upper() topic = pecan.request.rpcapi.get_topic_for_driver(driver_name) - ret, is_async = pecan.request.rpcapi.driver_vendor_passthru( + response = pecan.request.rpcapi.driver_vendor_passthru( pecan.request.context, driver_name, method, http_method, data, topic=topic) - status_code = 202 if is_async else 200 - return wsme.api.Response(ret, status_code=status_code) + status_code = 202 if response['async'] else 200 + return wsme.api.Response(response['return'], status_code=status_code) class DriversController(rest.RestController): diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 648e2b49e1..b7b05c38e0 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -712,11 +712,11 @@ class NodeVendorPassthruController(rest.RestController): data = {} http_method = pecan.request.method.upper() - ret, is_async = pecan.request.rpcapi.vendor_passthru( + response = pecan.request.rpcapi.vendor_passthru( pecan.request.context, rpc_node.uuid, method, http_method, data, topic) - status_code = 202 if is_async else 200 - return wsme.api.Response(ret, status_code=status_code) + status_code = 202 if response['async'] else 200 + return wsme.api.Response(response['return'], status_code=status_code) class NodeMaintenanceController(rest.RestController): diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index ee0fba43be..eedbd1f2ba 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -202,7 +202,7 @@ class ConductorManager(periodic_task.PeriodicTasks): """Ironic Conductor manager main class.""" # NOTE(rloo): This must be in sync with rpcapi.ConductorAPI's. - RPC_API_VERSION = '1.28' + RPC_API_VERSION = '1.29' target = messaging.Target(version=RPC_API_VERSION) @@ -449,11 +449,13 @@ class ConductorManager(periodic_task.PeriodicTasks): :raises: NoFreeConductorWorker when there is no free worker to start async task. :raises: NodeLocked if node is locked by another conductor. - :returns: A tuple containing the response of the invoked method - and a boolean value indicating whether the method was - invoked asynchronously (True) or synchronously (False). - If invoked asynchronously the response field will be - always None. + :returns: A dictionary containing: + + :return: The response of the invoked vendor method + :async: Boolean value. Whether the method was invoked + asynchronously (True) or synchronously (False). When invoked + asynchronously the response will be always None. + """ LOG.debug("RPC vendor_passthru called for node %s." % node_id) # NOTE(max_lobur): Even though not all vendor_passthru calls may @@ -495,7 +497,8 @@ class ConductorManager(periodic_task.PeriodicTasks): else: ret = vendor_func(task, **info) - return (ret, is_async) + return {'return': ret, + 'async': is_async} @messaging.expected_exceptions(exception.NoFreeConductorWorker, exception.InvalidParameterValue, @@ -526,11 +529,13 @@ class ConductorManager(periodic_task.PeriodicTasks): :raises: DriverNotFound if the supplied driver is not loaded. :raises: NoFreeConductorWorker when there is no free worker to start async task. - :returns: A tuple containing the response of the invoked method - and a boolean value indicating whether the method was - invoked asynchronously (True) or synchronously (False). - If invoked asynchronously the response field will be - always None. + :returns: A dictionary containing: + + :return: The response of the invoked vendor method + :async: Boolean value. Whether the method was invoked + asynchronously (True) or synchronously (False). When invoked + asynchronously the response will be always None. + """ # Any locking in a top-level vendor action will need to be done by the # implementation, as there is little we could reasonably lock on here. @@ -586,7 +591,8 @@ class ConductorManager(periodic_task.PeriodicTasks): else: ret = vendor_func(context, **info) - return (ret, is_async) + return {'return': ret, + 'async': is_async} @messaging.expected_exceptions(exception.UnsupportedDriverExtension) def get_node_vendor_passthru_methods(self, context, node_id): diff --git a/ironic/conductor/rpcapi.py b/ironic/conductor/rpcapi.py index 6cd65651e4..94b6950e3f 100644 --- a/ironic/conductor/rpcapi.py +++ b/ironic/conductor/rpcapi.py @@ -71,11 +71,13 @@ class ConductorAPI(object): | 1.26 - Added continue_node_clean | 1.27 - Convert continue_node_clean to cast | 1.28 - Change exceptions raised by destroy_node + | 1.29 - Change return value of vendor_passthru and + | driver_vendor_passthru to a dictionary """ # NOTE(rloo): This must be in sync with manager.ConductorManager's. - RPC_API_VERSION = '1.28' + RPC_API_VERSION = '1.29' def __init__(self, topic=None): super(ConductorAPI, self).__init__() @@ -190,11 +192,12 @@ class ConductorAPI(object): :raises: NoFreeConductorWorker when there is no free worker to start async task. :raises: NodeLocked if node is locked by another conductor. - :returns: A tuple containing the response of the invoked method - and a boolean value indicating whether the method was - invoked asynchronously (True) or synchronously (False). - If invoked asynchronously the response field will be - always None. + :returns: A dictionary containing: + + :return: The response of the invoked vendor method + :async: Boolean value. Whether the method was invoked + asynchronously (True) or synchronously (False). When invoked + asynchronously the response will be always None. """ cctxt = self.client.prepare(topic=topic or self.topic, version='1.20') @@ -226,11 +229,12 @@ class ConductorAPI(object): :raises: DriverNotFound if the supplied driver is not loaded. :raises: NoFreeConductorWorker when there is no free worker to start async task. - :returns: A tuple containing the response of the invoked method - and a boolean value indicating whether the method was - invoked asynchronously (True) or synchronously (False). - If invoked asynchronously the response field will be - always None. + :returns: A dictionary containing: + + :return: The response of the invoked vendor method + :async: Boolean value. Whether the method was invoked + asynchronously (True) or synchronously (False). When invoked + asynchronously the response will be always None. """ cctxt = self.client.prepare(topic=topic or self.topic, version='1.20') diff --git a/ironic/tests/api/v1/test_drivers.py b/ironic/tests/api/v1/test_drivers.py index 5cf51feb86..b4a1fbd148 100644 --- a/ironic/tests/api/v1/test_drivers.py +++ b/ironic/tests/api/v1/test_drivers.py @@ -76,55 +76,56 @@ class TestListDrivers(base.FunctionalTest): @mock.patch.object(rpcapi.ConductorAPI, 'driver_vendor_passthru') def test_driver_vendor_passthru_sync(self, mocked_driver_vendor_passthru): self.register_fake_conductors() - mocked_driver_vendor_passthru.return_value = ({ - 'return_key': 'return_value', - }, False) + mocked_driver_vendor_passthru.return_value = { + 'return': {'return_key': 'return_value'}, + 'async': False} response = self.post_json( '/drivers/%s/vendor_passthru/do_test' % self.d1, {'test_key': 'test_value'}) self.assertEqual(200, response.status_int) - self.assertEqual(mocked_driver_vendor_passthru.return_value[0], + self.assertEqual(mocked_driver_vendor_passthru.return_value['return'], response.json) @mock.patch.object(rpcapi.ConductorAPI, 'driver_vendor_passthru') def test_driver_vendor_passthru_async(self, mocked_driver_vendor_passthru): self.register_fake_conductors() - mocked_driver_vendor_passthru.return_value = (None, True) + mocked_driver_vendor_passthru.return_value = {'return': None, + 'async': True} response = self.post_json( '/drivers/%s/vendor_passthru/do_test' % self.d1, {'test_key': 'test_value'}) self.assertEqual(202, response.status_int) - self.assertIsNone(mocked_driver_vendor_passthru.return_value[0]) + self.assertIsNone(mocked_driver_vendor_passthru.return_value['return']) @mock.patch.object(rpcapi.ConductorAPI, 'driver_vendor_passthru') def test_driver_vendor_passthru_put(self, mocked_driver_vendor_passthru): self.register_fake_conductors() - return_value = (None, 'async') + return_value = {'return': None, 'async': True} mocked_driver_vendor_passthru.return_value = return_value response = self.put_json( '/drivers/%s/vendor_passthru/do_test' % self.d1, {'test_key': 'test_value'}) self.assertEqual(202, response.status_int) - self.assertEqual(return_value[0], response.json) + self.assertEqual(return_value['return'], response.json) @mock.patch.object(rpcapi.ConductorAPI, 'driver_vendor_passthru') def test_driver_vendor_passthru_get(self, mocked_driver_vendor_passthru): self.register_fake_conductors() - return_value = ('foo', 'sync') + return_value = {'return': 'foo', 'async': False} mocked_driver_vendor_passthru.return_value = return_value response = self.get_json( '/drivers/%s/vendor_passthru/do_test' % self.d1) - self.assertEqual(return_value[0], response) + self.assertEqual(return_value['return'], response) @mock.patch.object(rpcapi.ConductorAPI, 'driver_vendor_passthru') def test_driver_vendor_passthru_delete(self, mock_driver_vendor_passthru): self.register_fake_conductors() - return_value = (None, 'async') + return_value = {'return': None, 'async': True} mock_driver_vendor_passthru.return_value = return_value response = self.delete( '/drivers/%s/vendor_passthru/do_test' % self.d1) self.assertEqual(202, response.status_int) - self.assertEqual(return_value[0], response.json) + self.assertEqual(return_value['return'], response.json) def test_driver_vendor_passthru_driver_not_found(self): # tests when given driver is not found diff --git a/ironic/tests/api/v1/test_nodes.py b/ironic/tests/api/v1/test_nodes.py index d7cedf07f1..b138add20c 100644 --- a/ironic/tests/api/v1/test_nodes.py +++ b/ironic/tests/api/v1/test_nodes.py @@ -1199,7 +1199,7 @@ class TestPost(test_api_base.FunctionalTest): node = obj_utils.create_test_node(self.context) info = {'foo': 'bar'} - mock_vendor.return_value = (return_value, is_async) + mock_vendor.return_value = {'return': return_value, 'async': is_async} response = self.post_json('/nodes/%s/vendor_passthru/test' % node.uuid, info) mock_vendor.assert_called_once_with( @@ -1216,7 +1216,7 @@ class TestPost(test_api_base.FunctionalTest): node = obj_utils.create_test_node(self.context, name='node-109') info = {'foo': 'bar'} - mock_vendor.return_value = (return_value, is_async) + mock_vendor.return_value = {'return': return_value, 'async': is_async} response = self.post_json('/nodes/%s/vendor_passthru/test' % node.name, info, headers={api_base.Version.string: "1.5"}) @@ -1238,13 +1238,13 @@ class TestPost(test_api_base.FunctionalTest): @mock.patch.object(rpcapi.ConductorAPI, 'vendor_passthru') def test_vendor_passthru_put(self, mocked_vendor_passthru): node = obj_utils.create_test_node(self.context) - return_value = (None, 'async') + return_value = {'return': None, 'async': True} mocked_vendor_passthru.return_value = return_value response = self.put_json( '/nodes/%s/vendor_passthru/do_test' % node.uuid, {'test_key': 'test_value'}) self.assertEqual(202, response.status_int) - self.assertEqual(return_value[0], response.json) + self.assertEqual(return_value['return'], response.json) @mock.patch.object(rpcapi.ConductorAPI, 'vendor_passthru') def test_vendor_passthru_by_name(self, mock_vendor): @@ -1253,21 +1253,21 @@ class TestPost(test_api_base.FunctionalTest): @mock.patch.object(rpcapi.ConductorAPI, 'vendor_passthru') def test_vendor_passthru_get(self, mocked_vendor_passthru): node = obj_utils.create_test_node(self.context) - return_value = ('foo', 'sync') + return_value = {'return': 'foo', 'async': False} mocked_vendor_passthru.return_value = return_value response = self.get_json( '/nodes/%s/vendor_passthru/do_test' % node.uuid) - self.assertEqual(return_value[0], response) + self.assertEqual(return_value['return'], response) @mock.patch.object(rpcapi.ConductorAPI, 'vendor_passthru') def test_vendor_passthru_delete(self, mock_vendor_passthru): node = obj_utils.create_test_node(self.context) - return_value = (None, 'async') + return_value = {'return': None, 'async': True} mock_vendor_passthru.return_value = return_value response = self.delete( '/nodes/%s/vendor_passthru/do_test' % node.uuid) self.assertEqual(202, response.status_int) - self.assertEqual(return_value[0], response.json) + self.assertEqual(return_value['return'], response.json) def test_vendor_passthru_no_such_method(self): node = obj_utils.create_test_node(self.context) diff --git a/ironic/tests/conductor/test_manager.py b/ironic/tests/conductor/test_manager.py index 95e959e7e8..b7e0233c05 100644 --- a/ironic/tests/conductor/test_manager.py +++ b/ironic/tests/conductor/test_manager.py @@ -564,16 +564,16 @@ class VendorPassthruTestCase(_ServiceSetUpMixin, tests_db_base.DbTestCase): info = {'bar': 'baz'} self._start_service() - ret, is_async = self.service.vendor_passthru(self.context, node.uuid, - 'first_method', 'POST', - info) + response = self.service.vendor_passthru(self.context, node.uuid, + 'first_method', 'POST', + info) # Waiting to make sure the below assertions are valid. self.service._worker_pool.waitall() # Assert spawn_after was called self.assertTrue(mock_spawn.called) - self.assertIsNone(ret) - self.assertTrue(is_async) + self.assertIsNone(response['return']) + self.assertTrue(response['async']) node.refresh() self.assertIsNone(node.last_error) @@ -586,16 +586,16 @@ class VendorPassthruTestCase(_ServiceSetUpMixin, tests_db_base.DbTestCase): info = {'bar': 'meow'} self._start_service() - ret, is_async = self.service.vendor_passthru(self.context, node.uuid, - 'third_method_sync', - 'POST', info) + response = self.service.vendor_passthru(self.context, node.uuid, + 'third_method_sync', + 'POST', info) # Waiting to make sure the below assertions are valid. self.service._worker_pool.waitall() # Assert no workers were used self.assertFalse(mock_spawn.called) - self.assertTrue(ret) - self.assertFalse(is_async) + self.assertTrue(response['return']) + self.assertFalse(response['async']) node.refresh() self.assertIsNone(node.last_error) @@ -754,14 +754,14 @@ class VendorPassthruTestCase(_ServiceSetUpMixin, tests_db_base.DbTestCase): mock_spawn.reset_mock() vendor_args = {'test': 'arg'} - got, is_async = self.service.driver_vendor_passthru( + response = self.service.driver_vendor_passthru( self.context, 'fake', 'test_method', 'POST', vendor_args) # Assert that the vendor interface has no custom # driver_vendor_passthru() self.assertFalse(hasattr(self.driver.vendor, 'driver_vendor_passthru')) - self.assertEqual(expected, got) - self.assertFalse(is_async) + self.assertEqual(expected, response['return']) + self.assertFalse(response['async']) test_method.assert_called_once_with(self.context, **vendor_args) # No worker was spawned self.assertFalse(mock_spawn.called) @@ -779,14 +779,14 @@ class VendorPassthruTestCase(_ServiceSetUpMixin, tests_db_base.DbTestCase): mock_spawn.reset_mock() vendor_args = {'test': 'arg'} - got, is_async = self.service.driver_vendor_passthru( + response = self.service.driver_vendor_passthru( self.context, 'fake', 'test_sync_method', 'POST', vendor_args) # Assert that the vendor interface has no custom # driver_vendor_passthru() self.assertFalse(hasattr(self.driver.vendor, 'driver_vendor_passthru')) - self.assertIsNone(got) - self.assertTrue(is_async) + self.assertIsNone(response['return']) + self.assertTrue(response['async']) mock_spawn.assert_called_once_with(test_method, self.context, **vendor_args)