From a6530b486ac0a5d594ec4a103e9eb692330ae2b5 Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Thu, 28 Jan 2016 20:58:56 +0000 Subject: [PATCH] No need to have ironicclient parameter in methods An IronicDriver object has access to the ironic client via its 'ironicclient' member variable. This removes ironicclient parameters from methods and changes those methods to use the object's ironicclient member variable instead. It also moves a method into the class to take advantage of this. Change-Id: If4777d4ca858e92752938d588ec137a91c3216ad --- nova/tests/unit/virt/ironic/test_driver.py | 125 +++++++++++---------- nova/virt/ironic/driver.py | 67 +++++------ 2 files changed, 95 insertions(+), 97 deletions(-) diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 3c86333c5af2..8d86901e287f 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -135,25 +135,25 @@ class IronicDriverTestCase(test.NoDBTestCase): instance_uuid=self.instance_uuid) instance = fake_instance.fake_instance_obj(self.ctx, uuid=self.instance_uuid) - ironicclient = cw.IronicClientWrapper() - mock_gbiui.return_value = node - result = ironic_driver._validate_instance_and_node(ironicclient, - instance) + result = self.driver._validate_instance_and_node(instance) self.assertEqual(result.uuid, node_uuid) + mock_gbiui.assert_called_once_with(instance.uuid, + fields=ironic_driver._NODE_FIELDS) @mock.patch.object(FAKE_CLIENT.node, 'get_by_instance_uuid') def test__validate_instance_and_node_failed(self, mock_gbiui): - ironicclient = cw.IronicClientWrapper() mock_gbiui.side_effect = ironic_exception.NotFound() instance = fake_instance.fake_instance_obj(self.ctx, uuid=self.instance_uuid) self.assertRaises(exception.InstanceNotFound, - ironic_driver._validate_instance_and_node, - ironicclient, instance) + self.driver._validate_instance_and_node, instance) + mock_gbiui.assert_called_once_with(instance.uuid, + fields=ironic_driver._NODE_FIELDS) @mock.patch.object(objects.Instance, 'refresh') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') def test__wait_for_active_pass(self, fake_validate, fake_refresh): instance = fake_instance.fake_instance_obj(self.ctx, uuid=uuidutils.generate_uuid()) @@ -161,12 +161,13 @@ class IronicDriverTestCase(test.NoDBTestCase): provision_state=ironic_states.DEPLOYING) fake_validate.return_value = node - self.driver._wait_for_active(FAKE_CLIENT, instance) - fake_validate.assert_called_once_with(FAKE_CLIENT, instance) + self.driver._wait_for_active(instance) + fake_validate.assert_called_once_with(instance) fake_refresh.assert_called_once_with() @mock.patch.object(objects.Instance, 'refresh') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') def test__wait_for_active_done(self, fake_validate, fake_refresh): instance = fake_instance.fake_instance_obj(self.ctx, uuid=uuidutils.generate_uuid()) @@ -175,13 +176,13 @@ class IronicDriverTestCase(test.NoDBTestCase): fake_validate.return_value = node self.assertRaises(loopingcall.LoopingCallDone, - self.driver._wait_for_active, - FAKE_CLIENT, instance) - fake_validate.assert_called_once_with(FAKE_CLIENT, instance) + self.driver._wait_for_active, instance) + fake_validate.assert_called_once_with(instance) fake_refresh.assert_called_once_with() @mock.patch.object(objects.Instance, 'refresh') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') def test__wait_for_active_fail(self, fake_validate, fake_refresh): instance = fake_instance.fake_instance_obj(self.ctx, uuid=uuidutils.generate_uuid()) @@ -190,21 +191,20 @@ class IronicDriverTestCase(test.NoDBTestCase): fake_validate.return_value = node self.assertRaises(exception.InstanceDeployFailure, - self.driver._wait_for_active, - FAKE_CLIENT, instance) - fake_validate.assert_called_once_with(FAKE_CLIENT, instance) + self.driver._wait_for_active, instance) + fake_validate.assert_called_once_with(instance) fake_refresh.assert_called_once_with() @mock.patch.object(objects.Instance, 'refresh') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') def _wait_for_active_abort(self, instance_params, fake_validate, fake_refresh): instance = fake_instance.fake_instance_obj(self.ctx, uuid=uuidutils.generate_uuid(), **instance_params) self.assertRaises(exception.InstanceDeployFailure, - self.driver._wait_for_active, - FAKE_CLIENT, instance) + self.driver._wait_for_active, instance) # Assert _validate_instance_and_node wasn't called self.assertFalse(fake_validate.called) fake_refresh.assert_called_once_with() @@ -218,7 +218,8 @@ class IronicDriverTestCase(test.NoDBTestCase): def test__wait_for_active_abort_error(self): self._wait_for_active_abort({'vm_state': vm_states.ERROR}) - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') def test__wait_for_power_state_pass(self, fake_validate): instance = fake_instance.fake_instance_obj(self.ctx, uuid=uuidutils.generate_uuid()) @@ -226,11 +227,11 @@ class IronicDriverTestCase(test.NoDBTestCase): target_power_state=ironic_states.POWER_OFF) fake_validate.return_value = node - self.driver._wait_for_power_state( - FAKE_CLIENT, instance, 'fake message') + self.driver._wait_for_power_state(instance, 'fake message') self.assertTrue(fake_validate.called) - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') def test__wait_for_power_state_ok(self, fake_validate): instance = fake_instance.fake_instance_obj(self.ctx, uuid=uuidutils.generate_uuid()) @@ -239,8 +240,7 @@ class IronicDriverTestCase(test.NoDBTestCase): fake_validate.return_value = node self.assertRaises(loopingcall.LoopingCallDone, - self.driver._wait_for_power_state, - FAKE_CLIENT, instance, 'fake message') + self.driver._wait_for_power_state, instance, 'fake message') self.assertTrue(fake_validate.called) def _test__node_resource(self, has_inst_info): @@ -842,7 +842,6 @@ class IronicDriverTestCase(test.NoDBTestCase): self.assertFalse(mock_save.called) mock_looping.assert_called_once_with(mock_wait_active, - FAKE_CLIENT_WRAPPER, instance) fake_looping_call.start.assert_called_once_with( interval=CONF.ironic.api_retry_interval) @@ -964,7 +963,8 @@ class IronicDriverTestCase(test.NoDBTestCase): expected_patch = [{'path': '/instance_uuid', 'op': 'remove'}] mock_update.assert_called_once_with(node.uuid, expected_patch) - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') @mock.patch.object(FAKE_CLIENT.node, 'update') def test__cleanup_deploy_instance_already_removed(self, mock_update, mock_validate): @@ -979,7 +979,7 @@ class IronicDriverTestCase(test.NoDBTestCase): flavor=flavor) # assert node.update is not called self.assertFalse(mock_update.called) - mock_validate.assert_called_once_with(mock.ANY, instance) + mock_validate.assert_called_once_with(instance) @mock.patch.object(FAKE_CLIENT.node, 'update') def test__cleanup_deploy_without_flavor(self, mock_update): @@ -1211,7 +1211,8 @@ class IronicDriverTestCase(test.NoDBTestCase): self._test_destroy(state) @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') def test_destroy_trigger_undeploy_fail(self, fake_validate, mock_sps): node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' node = ironic_utils.get_test_node(driver='fake', uuid=node_uuid, @@ -1223,19 +1224,19 @@ class IronicDriverTestCase(test.NoDBTestCase): self.assertRaises(exception.NovaException, self.driver.destroy, self.ctx, instance, None, None) - @mock.patch.object(ironic_driver, '_validate_instance_and_node') - def _test__unprovision_instance(self, mock_validate_inst, state=None): - fake_ironic_client = mock.Mock() + @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') + def _test__unprovision_instance(self, mock_validate_inst, mock_set_pstate, + state=None): node = ironic_utils.get_test_node( driver='fake', provision_state=state) instance = fake_instance.fake_instance_obj(self.ctx, node=node.uuid) mock_validate_inst.return_value = node - self.driver._unprovision(fake_ironic_client, instance, node) - mock_validate_inst.assert_called_once_with(fake_ironic_client, - instance) - fake_ironic_client.call.assert_called_once_with( - "node.set_provision_state", node.uuid, "deleted") + self.driver._unprovision(instance, node) + mock_validate_inst.assert_called_once_with(instance) + mock_set_pstate.assert_called_once_with(node.uuid, "deleted") def test__unprovision_cleaning(self): self._test__unprovision_instance(state=ironic_states.CLEANING) @@ -1243,10 +1244,12 @@ class IronicDriverTestCase(test.NoDBTestCase): def test__unprovision_cleanwait(self): self._test__unprovision_instance(state=ironic_states.CLEANWAIT) - @mock.patch.object(ironic_driver, '_validate_instance_and_node') - def test__unprovision_fail_max_retries(self, mock_validate_inst): + @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') + def test__unprovision_fail_max_retries(self, mock_validate_inst, + mock_set_pstate): CONF.set_default('api_max_retries', default=2, group='ironic') - fake_ironic_client = mock.Mock() node = ironic_utils.get_test_node( driver='fake', provision_state=ironic_states.ACTIVE) @@ -1254,27 +1257,26 @@ class IronicDriverTestCase(test.NoDBTestCase): mock_validate_inst.return_value = node self.assertRaises(exception.NovaException, self.driver._unprovision, - fake_ironic_client, instance, node) - expected_calls = (mock.call(mock.ANY, instance), - mock.call(mock.ANY, instance)) + instance, node) + expected_calls = (mock.call(instance), + mock.call(instance)) mock_validate_inst.assert_has_calls(expected_calls) - fake_ironic_client.call.assert_called_once_with( - "node.set_provision_state", node.uuid, "deleted") + mock_set_pstate.assert_called_once_with(node.uuid, "deleted") - @mock.patch.object(ironic_driver, '_validate_instance_and_node') - def test__unprovision_instance_not_found(self, mock_validate_inst): - fake_ironic_client = mock.Mock() + @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') + def test__unprovision_instance_not_found(self, mock_validate_inst, + mock_set_pstate): node = ironic_utils.get_test_node( driver='fake', provision_state=ironic_states.DELETING) instance = fake_instance.fake_instance_obj(self.ctx, node=node.uuid) mock_validate_inst.side_effect = exception.InstanceNotFound( instance_id='fake') - self.driver._unprovision(fake_ironic_client, instance, node) - mock_validate_inst.assert_called_once_with(fake_ironic_client, - instance) - fake_ironic_client.call.assert_called_once_with( - "node.set_provision_state", node.uuid, "deleted") + self.driver._unprovision(instance, node) + mock_validate_inst.assert_called_once_with(instance) + mock_set_pstate.assert_called_once_with(node.uuid, "deleted") @mock.patch.object(FAKE_CLIENT, 'node') def test_destroy_unassociate_fail(self, mock_node): @@ -1293,7 +1295,8 @@ class IronicDriverTestCase(test.NoDBTestCase): instance.uuid, fields=ironic_driver._NODE_FIELDS) @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') @mock.patch.object(FAKE_CLIENT.node, 'set_power_state') def test_reboot(self, mock_sp, fake_validate, mock_looping): node = ironic_utils.get_test_node() @@ -1307,14 +1310,16 @@ class IronicDriverTestCase(test.NoDBTestCase): mock_sp.assert_called_once_with(node.uuid, 'reboot') @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') @mock.patch.object(FAKE_CLIENT.node, 'set_power_state') def test_power_off(self, mock_sp, fake_validate, mock_looping): self._test_power_on_off(mock_sp, fake_validate, mock_looping, method_name='power_off') @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall') - @mock.patch.object(ironic_driver, '_validate_instance_and_node') + @mock.patch.object(ironic_driver.IronicDriver, + '_validate_instance_and_node') @mock.patch.object(FAKE_CLIENT.node, 'set_power_state') def test_power_on(self, mock_sp, fake_validate, mock_looping): self._test_power_on_off(mock_sp, fake_validate, mock_looping, @@ -1560,9 +1565,7 @@ class IronicDriverTestCase(test.NoDBTestCase): flavor, preserve) mock_set_pstate.assert_called_once_with(node_uuid, ironic_states.REBUILD) - mock_looping.assert_called_once_with(mock_wait_active, - FAKE_CLIENT_WRAPPER, - instance) + mock_looping.assert_called_once_with(mock_wait_active, instance) fake_looping_call.start.assert_called_once_with( interval=CONF.ironic.api_retry_interval) fake_looping_call.wait.assert_called_once_with() diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index f2ed93ba38a8..fe4bcdcc3bba 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -87,19 +87,6 @@ def map_power_state(state): return power_state.NOSTATE -def _validate_instance_and_node(ironicclient, instance): - """Get the node associated with the instance. - - Check with the Ironic service that this instance is associated with a - node, and return the node. - """ - try: - return ironicclient.call('node.get_by_instance_uuid', instance.uuid, - fields=_NODE_FIELDS) - except ironic.exc.NotFound: - raise exception.InstanceNotFound(instance_id=instance.uuid) - - def _get_nodes_supported_instances(cpu_arch=None): """Return supported instances for a node.""" if not cpu_arch: @@ -169,6 +156,18 @@ class IronicDriver(virt_driver.ComputeDriver): return self.ironicclient.call('node.get', node_uuid, fields=_NODE_FIELDS) + def _validate_instance_and_node(self, instance): + """Get the node associated with the instance. + + Check with the Ironic service that this instance is associated with a + node, and return the node. + """ + try: + return self.ironicclient.call('node.get_by_instance_uuid', + instance.uuid, fields=_NODE_FIELDS) + except ironic.exc.NotFound: + raise exception.InstanceNotFound(instance_id=instance.uuid) + def _node_resources_unavailable(self, node_obj): """Determine whether the node's resources are in an acceptable state. @@ -385,7 +384,7 @@ class IronicDriver(virt_driver.ComputeDriver): # on the next cycle (M). patch.append({'op': 'remove', 'path': '/instance_uuid'}) try: - _validate_instance_and_node(self.ironicclient, instance) + self._validate_instance_and_node(instance) self.ironicclient.call('node.update', node.uuid, patch) except exception.InstanceNotFound: LOG.debug("Instance already removed from Ironic node %s. Skip " @@ -400,7 +399,7 @@ class IronicDriver(virt_driver.ComputeDriver): self._unplug_vifs(node, instance, network_info) self._stop_firewall(instance, network_info) - def _wait_for_active(self, ironicclient, instance): + def _wait_for_active(self, instance): """Wait for the node to be marked as ACTIVE in Ironic.""" instance.refresh() if (instance.task_state == task_states.DELETING or @@ -408,7 +407,7 @@ class IronicDriver(virt_driver.ComputeDriver): raise exception.InstanceDeployFailure( _("Instance %s provisioning was aborted") % instance.uuid) - node = _validate_instance_and_node(ironicclient, instance) + node = self._validate_instance_and_node(instance) if node.provision_state == ironic_states.ACTIVE: # job is done LOG.debug("Ironic node %(node)s is now ACTIVE", @@ -433,9 +432,9 @@ class IronicDriver(virt_driver.ComputeDriver): _log_ironic_polling('become ACTIVE', node, instance) - def _wait_for_power_state(self, ironicclient, instance, message): + def _wait_for_power_state(self, instance, message): """Wait for the node to complete a power state change.""" - node = _validate_instance_and_node(ironicclient, instance) + node = self._validate_instance_and_node(instance) if node.target_power_state == ironic_states.NOSTATE: raise loopingcall.LoopingCallDone() @@ -469,7 +468,7 @@ class IronicDriver(virt_driver.ComputeDriver): """ try: - _validate_instance_and_node(self.ironicclient, instance) + self._validate_instance_and_node(instance) return True except exception.InstanceNotFound: return False @@ -608,7 +607,7 @@ class IronicDriver(virt_driver.ComputeDriver): :returns: a InstanceInfo object """ try: - node = _validate_instance_and_node(self.ironicclient, instance) + node = self._validate_instance_and_node(instance) except exception.InstanceNotFound: return hardware.InstanceInfo( state=map_power_state(ironic_states.NOSTATE)) @@ -791,7 +790,6 @@ class IronicDriver(virt_driver.ComputeDriver): flavor=flavor) timer = loopingcall.FixedIntervalLoopingCall(self._wait_for_active, - self.ironicclient, instance) try: timer.start(interval=CONF.ironic.api_retry_interval).wait() @@ -804,12 +802,13 @@ class IronicDriver(virt_driver.ComputeDriver): {'instance': instance.uuid, 'node': node_uuid}) - def _unprovision(self, ironicclient, instance, node): + def _unprovision(self, instance, node): """This method is called from destroy() to unprovision already provisioned node after required checks. """ try: - ironicclient.call("node.set_provision_state", node.uuid, "deleted") + self.ironicclient.call("node.set_provision_state", node.uuid, + "deleted") except Exception as e: # if the node is already in a deprovisioned state, continue # This should be fixed in Ironic. @@ -824,7 +823,7 @@ class IronicDriver(virt_driver.ComputeDriver): def _wait_for_provision_state(): try: - node = _validate_instance_and_node(ironicclient, instance) + node = self._validate_instance_and_node(instance) except exception.InstanceNotFound: LOG.debug("Instance already removed from Ironic", instance=instance) @@ -875,7 +874,7 @@ class IronicDriver(virt_driver.ComputeDriver): """ LOG.debug('Destroy called for instance', instance=instance) try: - node = _validate_instance_and_node(self.ironicclient, instance) + node = self._validate_instance_and_node(instance) except exception.InstanceNotFound: LOG.warning(_LW("Destroy called on non-existing instance %s."), instance.uuid) @@ -886,7 +885,7 @@ class IronicDriver(virt_driver.ComputeDriver): return if node.provision_state in _UNPROVISION_STATES: - self._unprovision(self.ironicclient, instance, node) + self._unprovision(instance, node) self._cleanup_deploy(context, node, instance, network_info) LOG.info(_LI('Successfully unprovisioned Ironic node %s'), @@ -914,12 +913,11 @@ class IronicDriver(virt_driver.ComputeDriver): """ LOG.debug('Reboot called for instance', instance=instance) - node = _validate_instance_and_node(self.ironicclient, instance) + node = self._validate_instance_and_node(instance) self.ironicclient.call("node.set_power_state", node.uuid, 'reboot') timer = loopingcall.FixedIntervalLoopingCall( - self._wait_for_power_state, - self.ironicclient, instance, 'reboot') + self._wait_for_power_state, instance, 'reboot') timer.start(interval=CONF.ironic.api_retry_interval).wait() LOG.info(_LI('Successfully rebooted Ironic node %s'), node.uuid, instance=instance) @@ -939,12 +937,11 @@ class IronicDriver(virt_driver.ComputeDriver): for it to shutdown. Ignored by this driver. """ LOG.debug('Power off called for instance', instance=instance) - node = _validate_instance_and_node(self.ironicclient, instance) + node = self._validate_instance_and_node(instance) self.ironicclient.call("node.set_power_state", node.uuid, 'off') timer = loopingcall.FixedIntervalLoopingCall( - self._wait_for_power_state, - self.ironicclient, instance, 'power off') + self._wait_for_power_state, instance, 'power off') timer.start(interval=CONF.ironic.api_retry_interval).wait() LOG.info(_LI('Successfully powered off Ironic node %s'), node.uuid, instance=instance) @@ -965,12 +962,11 @@ class IronicDriver(virt_driver.ComputeDriver): """ LOG.debug('Power on called for instance', instance=instance) - node = _validate_instance_and_node(self.ironicclient, instance) + node = self._validate_instance_and_node(instance) self.ironicclient.call("node.set_power_state", node.uuid, 'on') timer = loopingcall.FixedIntervalLoopingCall( - self._wait_for_power_state, - self.ironicclient, instance, 'power on') + self._wait_for_power_state, instance, 'power on') timer.start(interval=CONF.ironic.api_retry_interval).wait() LOG.info(_LI('Successfully powered on Ironic node %s'), node.uuid, instance=instance) @@ -1163,7 +1159,6 @@ class IronicDriver(virt_driver.ComputeDriver): # Although the target provision state is REBUILD, it will actually go # to ACTIVE once the redeploy is finished. timer = loopingcall.FixedIntervalLoopingCall(self._wait_for_active, - self.ironicclient, instance) timer.start(interval=CONF.ironic.api_retry_interval).wait() LOG.info(_LI('Instance was successfully rebuilt'), instance=instance)