diff --git a/ironic/drivers/modules/agent_client.py b/ironic/drivers/modules/agent_client.py index 3ca44a1e16..ae8da4c9a7 100644 --- a/ironic/drivers/modules/agent_client.py +++ b/ironic/drivers/modules/agent_client.py @@ -19,6 +19,8 @@ import requests from ironic.common import exception from ironic.common.i18n import _ +from ironic.common.i18n import _LE +from ironic.common.i18n import _LW agent_opts = [ cfg.StrOpt('agent_api_version', @@ -32,6 +34,8 @@ CONF.register_opts(agent_opts, group='agent') LOG = log.getLogger(__name__) +DEFAULT_IPA_PORTAL_PORT = 3260 + class AgentClient(object): """Client for interacting with nodes via a REST API.""" @@ -127,7 +131,8 @@ class AgentClient(object): wait=wait) def start_iscsi_target(self, node, iqn, - portal_port=3260, wipe_disk_metadata=False): + portal_port=DEFAULT_IPA_PORTAL_PORT, + wipe_disk_metadata=False): """Expose the node's disk as an ISCSI target. :param node: an Ironic node object @@ -137,13 +142,59 @@ class AgentClient(object): disk magic strings like the partition table, RAID or filesystem signature. """ - params = {'iqn': iqn, - 'portal_port': portal_port, - 'wipe_disk_metadata': wipe_disk_metadata} - return self._command(node=node, - method='iscsi.start_iscsi_target', - params=params, - wait=True) + params = {'iqn': iqn} + # This is to workaround passing default values to an old ramdisk + # TODO(vdrok): remove this workaround in Ocata release + if portal_port != DEFAULT_IPA_PORTAL_PORT: + params['portal_port'] = portal_port + if wipe_disk_metadata: + params['wipe_disk_metadata'] = wipe_disk_metadata + while True: + result = self._command(node=node, + method='iscsi.start_iscsi_target', + params=params, + wait=True) + if (result['command_status'] == 'FAILED' and + result['command_error']['type'] == 'TypeError'): + message = result['command_error']['message'] + if 'wipe_disk_metadata' in message: + # wipe_disk_metadata was introduced after portal_port, so + # portal_port might still work, retry + LOG.warning(_LW( + "The ironic python agent in the ramdisk on node " + "%(node)s failed to start the iSCSI target because " + "it doesn't support wipe_disk_metadata parameter, " + "retrying without passing it. If you need to have " + "node's root disk wiped before exposing it via iSCSI, " + "or because https://bugs.launchpad.net/bugs/1550604 " + "affects you, please update the ramdisk to use " + "version >= 1.3 (Newton, or higher) of ironic python " + "agent."), {'node': node.uuid}) + # NOTE(vdrok): This is needed to make unit test's + # assert_has_calls work, otherwise it will report it was + # called without wipe_disk_metadata both times as "params" + # dictionary is stored by reference in mock + params = params.copy() + del params['wipe_disk_metadata'] + continue + elif 'portal_port' in message: + # It means that ironic is configured in a way that the + # deploy driver has requested some things not available + # on the old ramdisk. Since the user specified a + # non-default portal_port, we do not try again with the + # default value. Instead, the user needs to take some + # explicit action. + LOG.error(_LE( + "The ironic python agent in the ramdisk on node " + "%(node)s failed to start the iSCSI target because " + "the agent doesn't support portal_port parameter. " + "Please update the ramdisk to use version >= 1.3 " + "(Newton, or higher) of ironic python agent, or use " + "the default value of [iscsi]portal_port config " + "option."), {'node': node.uuid}) + # In all the other cases, it is a usual error, no additional action + # required, break from the loop returning the result + return result def install_bootloader(self, node, root_uuid, efi_system_part_uuid=None): """Install a boot loader on the image.""" diff --git a/ironic/tests/unit/drivers/modules/test_agent_client.py b/ironic/tests/unit/drivers/modules/test_agent_client.py index 4d71cafea9..0b1ed5d360 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_client.py +++ b/ironic/tests/unit/drivers/modules/test_agent_client.py @@ -141,7 +141,6 @@ class TestAgentClient(base.TestCase): mock_get.return_value = res self.assertEqual([], self.client.get_commands_status(self.node)) - @mock.patch('uuid.uuid4', mock.MagicMock(spec_set=[], return_value='uuid')) def test_prepare_image(self): self.client._command = mock.MagicMock(spec_set=[]) image_info = {'image_id': 'image'} @@ -154,7 +153,6 @@ class TestAgentClient(base.TestCase): node=self.node, method='standby.prepare_image', params=params, wait=False) - @mock.patch('uuid.uuid4', mock.MagicMock(spec_set=[], return_value='uuid')) def test_prepare_image_with_configdrive(self): self.client._command = mock.MagicMock(spec_set=[]) configdrive_url = 'http://swift/configdrive' @@ -172,19 +170,110 @@ class TestAgentClient(base.TestCase): node=self.node, method='standby.prepare_image', params=params, wait=False) - @mock.patch('uuid.uuid4', mock.MagicMock(spec_set=[], return_value='uuid')) def test_start_iscsi_target(self): self.client._command = mock.MagicMock(spec_set=[]) iqn = 'fake-iqn' port = 3260 - params = {'iqn': iqn, 'portal_port': port, 'wipe_disk_metadata': False} + wipe_disk_metadata = True + params = {'iqn': iqn, 'wipe_disk_metadata': True} - self.client.start_iscsi_target(self.node, iqn, port) + self.client.start_iscsi_target(self.node, iqn, portal_port=port, + wipe_disk_metadata=wipe_disk_metadata) self.client._command.assert_called_once_with( node=self.node, method='iscsi.start_iscsi_target', params=params, wait=True) - @mock.patch('uuid.uuid4', mock.MagicMock(spec_set=[], return_value='uuid')) + def test_start_iscsi_target_custom_port(self): + self.client._command = mock.MagicMock(spec_set=[]) + iqn = 'fake-iqn' + port = 3261 + wipe_disk_metadata = False + params = {'iqn': iqn, 'portal_port': port} + + self.client.start_iscsi_target(self.node, iqn, portal_port=port, + wipe_disk_metadata=wipe_disk_metadata) + self.client._command.assert_called_once_with( + node=self.node, method='iscsi.start_iscsi_target', + params=params, wait=True) + + @mock.patch.object(agent_client, 'LOG') + def test_start_iscsi_target_old_agent_only_wipe(self, log_mock): + self.client._command = mock.MagicMock( + spec_set=[], + side_effect=[ + { + 'command_status': 'FAILED', + 'command_error': { + 'message': u"wipe_disk_metadata doesn't exist", + 'type': u'TypeError' + } + }, + { + 'command_status': 'SUCCESS', + } + ] + ) + iqn = 'fake-iqn' + port = 3260 + wipe_disk_metadata = True + params_first_try = {'iqn': iqn, + 'wipe_disk_metadata': wipe_disk_metadata} + params_second_try = {'iqn': iqn} + + ret = self.client.start_iscsi_target( + self.node, iqn, portal_port=port, + wipe_disk_metadata=wipe_disk_metadata) + self.client._command.assert_has_calls([ + mock.call(node=self.node, method='iscsi.start_iscsi_target', + params=params_first_try, wait=True), + mock.call(node=self.node, method='iscsi.start_iscsi_target', + params=params_second_try, wait=True) + ]) + self.assertEqual('SUCCESS', ret['command_status']) + self.assertTrue(log_mock.warning.called) + self.assertFalse(log_mock.error.called) + + @mock.patch.object(agent_client, 'LOG') + def test_start_iscsi_target_old_agent_custom_options(self, log_mock): + self.client._command = mock.MagicMock( + spec_set=[], + side_effect=[ + { + 'command_status': 'FAILED', + 'command_error': { + 'message': u"wipe_disk_metadata doesn't exist", + 'type': u'TypeError' + } + }, + { + 'command_status': 'FAILED', + 'command_error': { + 'message': u"portal_port doesn't exist", + 'type': u'TypeError' + } + } + ] + ) + iqn = 'fake-iqn' + port = 3261 + wipe_disk_metadata = True + params_first_try = {'iqn': iqn, 'portal_port': port, + 'wipe_disk_metadata': wipe_disk_metadata} + params_second_try = {'iqn': iqn, 'portal_port': port} + + ret = self.client.start_iscsi_target( + self.node, iqn, portal_port=port, + wipe_disk_metadata=wipe_disk_metadata) + self.client._command.assert_has_calls([ + mock.call(node=self.node, method='iscsi.start_iscsi_target', + params=params_first_try, wait=True), + mock.call(node=self.node, method='iscsi.start_iscsi_target', + params=params_second_try, wait=True) + ]) + self.assertEqual('FAILED', ret['command_status']) + self.assertTrue(log_mock.warning.called) + self.assertTrue(log_mock.error.called) + def test_install_bootloader(self): self.client._command = mock.MagicMock(spec_set=[]) root_uuid = 'fake-root-uuid' diff --git a/releasenotes/notes/fix-mitaka-ipa-iscsi.yaml b/releasenotes/notes/fix-mitaka-ipa-iscsi.yaml new file mode 100644 index 0000000000..e68eac7da9 --- /dev/null +++ b/releasenotes/notes/fix-mitaka-ipa-iscsi.yaml @@ -0,0 +1,8 @@ +--- +upgrade: + - Fixed Mitaka ironic python agent ramdisk iSCSI deploy compatibility with + newer versions of ironic by logging the warning and retrying the deploy if + wiping root disk metadata before exposing it over iSCSI fails. If custom + iSCSI port is requested, an error clarifying the issue is logged and the + operator is requested either to use the default iSCSI portal port, or to + upgrade ironic python agent ramdisk to version >= 1.3 (Newton).