From 013ac6e504bb1b754c8de217c47bb65f80d9815a Mon Sep 17 00:00:00 2001 From: Patrick East Date: Fri, 9 Oct 2015 16:35:12 -0700 Subject: [PATCH] Add retries for Cisco FCZM client CLI _cfg_save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../test_cisco_fc_zone_client_cli.py | 60 ++++++++++++++++++- .../drivers/cisco/cisco_fc_zone_client_cli.py | 41 ++++--------- 2 files changed, 69 insertions(+), 32 deletions(-) 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 137d5dda9..57ca8e40b 100644 --- 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 705ba1805..4cdbda1ed 100644 --- 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.