Gracefully degrade start_iscsi_target for Mitaka ramdisk

Currently calling this code blows up if using an older ramdisk. If
default values for 'portal_port' or 'wipe_disk_metadata' parameters
are set, do not pass it to IPA to workaround this problem. If
start_iscsi_target fails and wipe_disk_metadata is True, try to
start iSCSI without it, if custom portal_port is requested, tell the
user that either ramdisk should be updated or default port should be
used.

Closes-Bug: #1584005
Co-Authored-By: Vladyslav Drok <vdrok@mirantis.com>
Change-Id: I319fb18bb8ff58970c219cc4f4dba7b54e239bd4

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,
+        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."""

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'

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).