Add retries for Cisco FCZM client CLI _cfg_save

Previously this config update would break if there were other changes
happening that were not completed yet. An easy work-around is to just
retry if it fails to apply the configuration update. There is already
code in place to handle this, but it doesn’t actually work correctly.

Instead of fixing it I’ve switched over to using the utils.retry
decorator to do the retries.

Change-Id: I3c1948bcfdedc633c23a30351260ce8fbf7342de
Closes-Bug: #1482398

diff --git a/cinder/tests/unit/zonemanager/test_cisco_fc_zone_client_cli.py b/cinder/tests/unit/zonemanager/test_cisco_fc_zone_client_cli.py
index 137d5dda944a7f46c132e378759af2bbf6d1b35e..57ca8e40b6b285003846ad583be7652058ed9fb1 100644 (file)
--- a/cinder/tests/unit/zonemanager/test_cisco_fc_zone_client_cli.py
+++ b/cinder/tests/unit/zonemanager/test_cisco_fc_zone_client_cli.py
@@ -192,7 +192,38 @@ class TestCiscoFCZoneClientCLI(cli.CiscoFCZoneClientCLI, test.TestCase):
     def test__cfg_save(self, run_ssh_mock):
         cmd_list = ['copy', 'running-config', 'startup-config']
         self._cfg_save()
-        run_ssh_mock.assert_called_once_with(cmd_list, True, 1)
+        run_ssh_mock.assert_called_once_with(cmd_list, True)
+
+    @mock.patch.object(cli.CiscoFCZoneClientCLI, '_run_ssh')
+    def test__cfg_save_with_retry(self, run_ssh_mock):
+        cmd_list = ['copy', 'running-config', 'startup-config']
+        run_ssh_mock.side_effect = [
+            processutils.ProcessExecutionError,
+            ('', None)
+        ]
+
+        self._cfg_save()
+
+        self.assertEqual(2, run_ssh_mock.call_count)
+        run_ssh_mock.assert_has_calls([
+            mock.call(cmd_list, True),
+            mock.call(cmd_list, True)
+        ])
+
+    @mock.patch.object(cli.CiscoFCZoneClientCLI, '_run_ssh')
+    def test__cfg_save_with_error(self, run_ssh_mock):
+        cmd_list = ['copy', 'running-config', 'startup-config']
+        run_ssh_mock.side_effect = processutils.ProcessExecutionError
+
+        self.assertRaises(processutils.ProcessExecutionError, self._cfg_save)
+
+        expected_num_calls = 5
+        expected_calls = []
+        for i in xrange(expected_num_calls):
+            expected_calls.append(mock.call(cmd_list, True))
+
+        self.assertEqual(expected_num_calls, run_ssh_mock.call_count)
+        run_ssh_mock.assert_has_calls(expected_calls)
 
     @mock.patch.object(cli.CiscoFCZoneClientCLI, '_run_ssh')
     def test__get_switch_info(self, run_ssh_mock):
@@ -201,7 +232,7 @@ class TestCiscoFCZoneClientCLI(cli.CiscoFCZoneClientCLI, test.TestCase):
         run_ssh_mock.return_value = (Stream(nsshow), Stream())
         switch_data = self._get_switch_info(cmd_list)
         self.assertEqual(nsshow_list, switch_data)
-        run_ssh_mock.assert_called_once_with(cmd_list, True, 1)
+        run_ssh_mock.assert_called_once_with(cmd_list, True)
 
     def test__parse_ns_output(self):
         return_wwn_list = []
@@ -210,6 +241,31 @@ class TestCiscoFCZoneClientCLI(cli.CiscoFCZoneClientCLI, test.TestCase):
         self.assertEqual(expected_wwn_list, return_wwn_list)
 
 
+class TestCiscoFCZoneClientCLISSH(test.TestCase):
+
+    def setUp(self):
+        super(TestCiscoFCZoneClientCLISSH, self).setUp()
+        self.client = cli.CiscoFCZoneClientCLI(None, None, None, None, None)
+        self.client.sshpool = mock.MagicMock()
+        self.mock_ssh = self.client.sshpool.item().__enter__()
+
+    @mock.patch('oslo_concurrency.processutils.ssh_execute')
+    def test__run_ssh(self, mock_execute):
+        mock_execute.return_value = 'ssh output'
+        ret = self.client._run_ssh(['cat', 'foo'])
+        self.assertEqual('ssh output', ret)
+        mock_execute.assert_called_once_with(self.mock_ssh,
+                                             'cat foo',
+                                             check_exit_code=True)
+
+    @mock.patch('oslo_concurrency.processutils.ssh_execute')
+    def test__run_ssh_with_error(self, mock_execute):
+        mock_execute.side_effect = processutils.ProcessExecutionError()
+        self.assertRaises(processutils.ProcessExecutionError,
+                          self.client._run_ssh,
+                          ['cat', 'foo'])
+
+
 class Channel(object):
     def recv_exit_status(self):
         return 0

diff --git a/cinder/zonemanager/drivers/cisco/cisco_fc_zone_client_cli.py b/cinder/zonemanager/drivers/cisco/cisco_fc_zone_client_cli.py
index 705ba1805f38691788b0ff0d036212c3aaebdcb2..4cdbda1ed9c0d876c46ffe544c74efad68d89a4f 100644 (file)
--- a/cinder/zonemanager/drivers/cisco/cisco_fc_zone_client_cli.py
+++ b/cinder/zonemanager/drivers/cisco/cisco_fc_zone_client_cli.py
@@ -28,7 +28,7 @@ from oslo_utils import excutils
 import six
 
 from cinder import exception
-from cinder.i18n import _, _LE, _LI
+from cinder.i18n import _, _LE, _LI, _LW
 from cinder import ssh_utils
 from cinder import utils
 import cinder.zonemanager.drivers.cisco.fc_zone_constants as ZoneConstant
@@ -313,14 +313,15 @@ class CiscoFCZoneClientCLI(object):
 
         return return_list
 
+    @utils.retry(processutils.ProcessExecutionError, retries=5)
     def _cfg_save(self):
         cmd = ['copy', 'running-config', 'startup-config']
-        self._run_ssh(cmd, True, 1)
+        self._run_ssh(cmd, True)
 
     def _get_switch_info(self, cmd_list):
         stdout, stderr, sw_data = None, None, None
         try:
-            stdout, stderr = self._run_ssh(cmd_list, True, 1)
+            stdout, stderr = self._run_ssh(cmd_list, True)
             LOG.debug("CLI output from ssh - output: %s", stdout)
             if (stdout):
                 sw_data = stdout.splitlines()
@@ -353,7 +354,7 @@ class CiscoFCZoneClientCLI(object):
                 raise exception.InvalidParameterValue(err=msg)
         return return_list
 
-    def _run_ssh(self, cmd_list, check_exit_code=True, attempts=1):
+    def _run_ssh(self, cmd_list, check_exit_code=True):
 
         command = ' '.join(cmd_list)
 
@@ -365,36 +366,16 @@ class CiscoFCZoneClientCLI(object):
                                              self.switch_pwd,
                                              min_size=1,
                                              max_size=5)
-        last_exception = None
         try:
             with self.sshpool.item() as ssh:
-                while attempts > 0:
-                    attempts -= 1
-                    try:
-                        return processutils.ssh_execute(
-                            ssh,
-                            command,
-                            check_exit_code=check_exit_code)
-                    except Exception as e:
-                        msg = _("Exception: %s") % six.text_type(e)
-                        LOG.error(msg)
-                        last_exception = e
-                        greenthread.sleep(random.randint(20, 500) / 100.0)
-                try:
-                    raise processutils.ProcessExecutionError(
-                        exit_code=last_exception.exit_code,
-                        stdout=last_exception.stdout,
-                        stderr=last_exception.stderr,
-                        cmd=last_exception.cmd)
-                except AttributeError:
-                    raise processutils.ProcessExecutionError(
-                        exit_code=-1,
-                        stdout="",
-                        stderr="Error running SSH command",
-                        cmd=command)
+                return processutils.ssh_execute(
+                    ssh,
+                    command,
+                    check_exit_code=check_exit_code)
+
         except Exception:
             with excutils.save_and_reraise_exception():
-                LOG.error(_LE("Error running SSH command: %s"), command)
+                LOG.warning(_LW("Error running SSH command: %s"), command)
 
     def _ssh_execute(self, cmd_list, check_exit_code=True, attempts=1):
         """Execute cli with status update.