From: Michael Price Date: Thu, 17 Sep 2015 20:33:37 +0000 (-0500) Subject: NetApp: Fix issue with updating E-Series password X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=1b4e2e7a48ec76fa5bc80b13fc4a04bce660ad31;p=openstack-build%2Fcinder-build.git NetApp: Fix issue with updating E-Series password The NetApp E-Series storage array password is specified in the configuration file, and must be updated if the array has its password changed via the management software. However, the logic to check whether or not the password is currently valid was incorrect, and was not correctly detecting an invalid password in need of updating. This patch fixes the password validity checking for the driver, refactors the existing code to be more concise, and adds additional unittests around this functionality. Change-Id: I1468d1810479947a9cff7ef8b79440de4063cbed Closes-Bug: #1497007 --- diff --git a/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py b/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py index 68b81fd8a..1dd6ef505 100644 --- a/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py +++ b/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py @@ -600,6 +600,7 @@ STORAGE_SYSTEM = { '1fa6efb5-f07b-4de4-9f0e-52e5f7ff5d1b', 'hotSpareSizeAsString': '0', 'wwn': '60080E500023C73400000000515AF323', + 'passwordStatus': 'valid', 'parameters': { 'minVolSize': 1048576, 'maxSnapshotsPerBase': 16, 'maxDrives': 192, @@ -1071,3 +1072,6 @@ class FakeEseriesClient(object): def list_target_wwpns(self, *args, **kwargs): return [WWPN_2] + + def update_stored_system_password(self, *args, **kwargs): + pass diff --git a/cinder/tests/unit/volume/drivers/netapp/eseries/test_client.py b/cinder/tests/unit/volume/drivers/netapp/eseries/test_client.py index dcd9fca5e..4c167ec8f 100644 --- a/cinder/tests/unit/volume/drivers/netapp/eseries/test_client.py +++ b/cinder/tests/unit/volume/drivers/netapp/eseries/test_client.py @@ -70,18 +70,21 @@ class NetAppEseriesClientDriverTestCase(test.TestCase): self.assertEqual(status_code, exc.status_code) - def test_eval_response_422(self): + @ddt.data(('30', 'storage array password.*?incorrect'), + ('authFailPassword', 'storage array password.*?incorrect'), + ('unknown', None)) + @ddt.unpack + def test_eval_response_422(self, ret_code, exc_regex): status_code = 422 - resp_text = "Fake Error Message" fake_resp = mock.Mock() + fake_resp.text = "fakeError" + fake_resp.json = mock.Mock(return_value={'retcode': ret_code}) fake_resp.status_code = status_code - fake_resp.text = resp_text - expected_msg = "Response error - %s." % resp_text + exc_regex = exc_regex if exc_regex is not None else fake_resp.text - with self.assertRaisesRegex(es_exception.WebServiceException, - expected_msg) as exc: + with self.assertRaisesRegexp(es_exception.WebServiceException, + exc_regex) as exc: self.my_client._eval_response(fake_resp) - self.assertEqual(status_code, exc.status_code) def test_register_storage_system_does_not_log_password(self): diff --git a/cinder/tests/unit/volume/drivers/netapp/eseries/test_library.py b/cinder/tests/unit/volume/drivers/netapp/eseries/test_library.py index de69391b6..2fcf9a773 100644 --- a/cinder/tests/unit/volume/drivers/netapp/eseries/test_library.py +++ b/cinder/tests/unit/volume/drivers/netapp/eseries/test_library.py @@ -19,6 +19,7 @@ import copy import ddt +import time import mock from oslo_utils import units @@ -26,7 +27,9 @@ import six from cinder import exception from cinder import test + from cinder.tests.unit import fake_snapshot +from cinder.tests.unit import utils as cinder_utils from cinder.tests.unit.volume.drivers.netapp.eseries import fakes as \ eseries_fake from cinder.volume.drivers.netapp.eseries import client as es_client @@ -63,7 +66,9 @@ class NetAppEseriesLibraryTestCase(test.TestCase): # Deprecated Option self.library.configuration.netapp_storage_pools = None self.library._client = eseries_fake.FakeEseriesClient() - self.library.check_for_setup_error() + with mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', + new = cinder_utils.ZeroIntervalLoopingCall): + self.library.check_for_setup_error() def test_do_setup(self): self.mock_object(self.library, @@ -75,13 +80,112 @@ class NetAppEseriesLibraryTestCase(test.TestCase): self.assertTrue(mock_check_flags.called) + @ddt.data(('optimal', True), ('offline', False), ('needsAttn', True), + ('neverContacted', False), ('newKey', True), (None, True)) + @ddt.unpack + @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', new= + cinder_utils.ZeroIntervalLoopingCall) + def test_check_storage_system_status(self, status, status_valid): + system = copy.deepcopy(eseries_fake.STORAGE_SYSTEM) + system['status'] = status + status = status.lower() if status is not None else '' + + actual_status, actual_valid = ( + self.library._check_storage_system_status(system)) + + self.assertEqual(status, actual_status) + self.assertEqual(status_valid, actual_valid) + + @ddt.data(('valid', True), ('invalid', False), ('unknown', False), + ('newKey', True), (None, True)) + @ddt.unpack + @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', new= + cinder_utils.ZeroIntervalLoopingCall) + def test_check_password_status(self, status, status_valid): + system = copy.deepcopy(eseries_fake.STORAGE_SYSTEM) + system['passwordStatus'] = status + status = status.lower() if status is not None else '' + + actual_status, actual_valid = ( + self.library._check_password_status(system)) + + self.assertEqual(status, actual_status) + self.assertEqual(status_valid, actual_valid) + + def test_check_storage_system_bad_system(self): + exc_str = "bad_system" + controller_ips = self.library.configuration.netapp_controller_ips + self.library._client.list_storage_system = mock.Mock( + side_effect=exception.NetAppDriverException(message=exc_str)) + info_log = self.mock_object(library.LOG, 'info', mock.Mock()) + + self.assertRaisesRegexp(exception.NetAppDriverException, exc_str, + self.library._check_storage_system) + + info_log.assert_called_once_with(mock.ANY, controller_ips) + + @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', new= + cinder_utils.ZeroIntervalLoopingCall) + def test_check_storage_system(self): + system = copy.deepcopy(eseries_fake.STORAGE_SYSTEM) + self.mock_object(self.library._client, 'list_storage_system', + new_attr=mock.Mock(return_value=system)) + update_password = self.mock_object(self.library._client, + 'update_stored_system_password') + info_log = self.mock_object(library.LOG, 'info', mock.Mock()) + + self.library._check_storage_system() + + self.assertTrue(update_password.called) + self.assertTrue(info_log.called) + + @ddt.data({'status': 'optimal', 'passwordStatus': 'invalid'}, + {'status': 'offline', 'passwordStatus': 'valid'}) + @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', new= + cinder_utils.ZeroIntervalLoopingCall) + def test_check_storage_system_bad_status(self, system): + self.mock_object(self.library._client, 'list_storage_system', + new_attr=mock.Mock(return_value=system)) + self.mock_object(self.library._client, 'update_stored_system_password') + self.mock_object(time, 'time', new_attr = mock.Mock( + side_effect=xrange(0, 60, 5))) + + self.assertRaisesRegexp(exception.NetAppDriverException, + 'bad.*?status', + self.library._check_storage_system) + + @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', new= + cinder_utils.ZeroIntervalLoopingCall) + def test_check_storage_system_update_password(self): + self.library.configuration.netapp_sa_password = 'password' + + def get_system_iter(): + key = 'passwordStatus' + system = copy.deepcopy(eseries_fake.STORAGE_SYSTEM) + system[key] = 'invalid' + yield system + yield system + + system[key] = 'valid' + yield system + + self.mock_object(self.library._client, 'list_storage_system', + new_attr=mock.Mock(side_effect=get_system_iter())) + update_password = self.mock_object(self.library._client, + 'update_stored_system_password', + new_attr=mock.Mock()) + info_log = self.mock_object(library.LOG, 'info', mock.Mock()) + + self.library._check_storage_system() + + update_password.assert_called_once_with( + self.library.configuration.netapp_sa_password) + self.assertTrue(info_log.called) + def test_get_storage_pools_empty_result(self): """Verify an exception is raised if no pools are returned.""" self.library.configuration.netapp_pool_name_search_pattern = '$' - self.assertRaises(exception.NetAppDriverException, - self.library.check_for_setup_error) - def test_get_storage_pools_invalid_conf(self): """Verify an exception is raised if the regex pattern is invalid.""" self.library.configuration.netapp_pool_name_search_pattern = '(.*' @@ -896,7 +1000,9 @@ class NetAppEseriesLibraryMultiAttachTestCase(test.TestCase): self.library = library.NetAppESeriesLibrary("FAKE", **kwargs) self.library._client = eseries_fake.FakeEseriesClient() - self.library.check_for_setup_error() + with mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', + new = cinder_utils.ZeroIntervalLoopingCall): + self.library.check_for_setup_error() def test_do_setup_host_group_already_exists(self): mock_check_flags = self.mock_object(na_utils, 'check_flags') diff --git a/cinder/volume/drivers/netapp/eseries/client.py b/cinder/volume/drivers/netapp/eseries/client.py index a97ced89d..6715820c1 100644 --- a/cinder/volume/drivers/netapp/eseries/client.py +++ b/cinder/volume/drivers/netapp/eseries/client.py @@ -267,6 +267,13 @@ class RestClient(WebserviceClient): # Response code 422 returns error code and message if status_code == 422: msg = _("Response error - %s.") % response.text + json_response = response.json() + if json_response is not None: + ret_code = json_response.get('retcode', '') + if ret_code == '30' or ret_code == 'authFailPassword': + msg = _("The storage array password for %s is " + "incorrect, please update the configured " + "password.") % self._system_id else: msg = _("Response error code - %s.") % status_code raise es_exception.WebServiceException(msg, diff --git a/cinder/volume/drivers/netapp/eseries/library.py b/cinder/volume/drivers/netapp/eseries/library.py index 2c90a5bda..bf996dbe0 100644 --- a/cinder/volume/drivers/netapp/eseries/library.py +++ b/cinder/volume/drivers/netapp/eseries/library.py @@ -109,6 +109,7 @@ class NetAppESeriesLibrary(object): RAID_UQ_SPEC = 'netapp_raid_type' THIN_UQ_SPEC = 'netapp_thin_provisioned' SSC_UPDATE_INTERVAL = 60 # seconds + SA_COMM_TIMEOUT = 30 WORLDWIDENAME = 'worldWideName' DEFAULT_HOST_TYPE = 'linux_dm_mp' @@ -242,48 +243,104 @@ class NetAppESeriesLibrary(object): self._client.set_system_id(system.get('id')) self._client._init_features() + def _check_password_status(self, system): + """Determine if the storage system's password status is valid. + + The password status has the following possible states: unknown, valid, + invalid. + + If the password state cannot be retrieved from the storage system, + an empty string will be returned as the status, and the password + status will be assumed to be valid. This is done to ensure that + access to a storage system will not be blocked in the event of a + problem with the API. + + This method returns a tuple consisting of the storage system's + password status and whether or not the status is valid. + + Example: (invalid, True) + + :returns (str, bool) + """ + + status = system.get('passwordStatus') + status = status.lower() if status else '' + return status, status not in ['invalid', 'unknown'] + + def _check_storage_system_status(self, system): + """Determine if the storage system's status is valid. + + The storage system status has the following possible states: + neverContacted, offline, optimal, needsAttn. + + If the storage system state cannot be retrieved, an empty string will + be returned as the status, and the storage system's status will be + assumed to be valid. This is done to ensure that access to a storage + system will not be blocked in the event of a problem with the API. + + This method returns a tuple consisting of the storage system's + password status and whether or not the status is valid. + + Example: (needsAttn, True) + + :returns (str, bool) + """ + status = system.get('status') + status = status.lower() if status else '' + return status, status not in ['nevercontacted', 'offline'] + def _check_storage_system(self): """Checks whether system is registered and has good status.""" try: - system = self._client.list_storage_system() + self._client.list_storage_system() except exception.NetAppDriverException: with excutils.save_and_reraise_exception(): LOG.info(_LI("System with controller addresses [%s] is not " "registered with web service."), self.configuration.netapp_controller_ips) - password_not_in_sync = False - if system.get('status', '').lower() == 'passwordoutofsync': - password_not_in_sync = True - new_pwd = self.configuration.netapp_sa_password - self._client.update_stored_system_password(new_pwd) - time.sleep(self.SLEEP_SECS) - sa_comm_timeout = 60 - comm_time = 0 - while True: + + # Update the stored password + # We do this to trigger the webservices password validation routine + new_pwd = self.configuration.netapp_sa_password + self._client.update_stored_system_password(new_pwd) + + start_time = int(time.time()) + + def check_system_status(): system = self._client.list_storage_system() - status = system.get('status', '').lower() + pass_status, pass_status_valid = ( + self._check_password_status(system)) + status, status_valid = self._check_storage_system_status(system) + msg_dict = {'id': system.get('id'), 'status': status, + 'pass_status': pass_status} # wait if array not contacted or # password was not in sync previously. - if ((status == 'nevercontacted') or - (password_not_in_sync and status == 'passwordoutofsync')): - LOG.info(_LI('Waiting for web service array communication.')) - time.sleep(self.SLEEP_SECS) - comm_time = comm_time + self.SLEEP_SECS - if comm_time >= sa_comm_timeout: - msg = _("Failure in communication between web service and" - " array. Waited %s seconds. Verify array" - " configuration parameters.") - raise exception.NetAppDriverException(msg % - sa_comm_timeout) + if not (pass_status_valid and status_valid): + if not pass_status_valid: + LOG.info(_LI('Waiting for web service to validate the ' + 'configured password.')) + else: + LOG.info(_LI('Waiting for web service array ' + 'communication.')) + if int(time.time() - start_time) >= self.SA_COMM_TIMEOUT: + if not status_valid: + raise exception.NetAppDriverException( + _("System %(id)s found with bad status - " + "%(status)s.") % msg_dict) + else: + raise exception.NetAppDriverException( + _("System %(id)s found with bad password status - " + "%(pass_status)s.") % msg_dict) + + # The system was found to have a good status else: - break - msg_dict = {'id': system.get('id'), 'status': status} - if (status == 'passwordoutofsync' or status == 'notsupported' or - status == 'offline'): - raise exception.NetAppDriverException( - _("System %(id)s found with bad status - " - "%(status)s.") % msg_dict) - LOG.info(_LI("System %(id)s has %(status)s status."), msg_dict) + LOG.info(_LI("System %(id)s has %(status)s status."), msg_dict) + raise loopingcall.LoopingCallDone() + + checker = loopingcall.FixedIntervalLoopingCall(f=check_system_status) + checker.start(interval = self.SLEEP_SECS, + initial_delay=self.SLEEP_SECS).wait() + return True def _get_volume(self, uid):