From 5821f8b8714c532778f2eef142a5fdeb3a1e6f05 Mon Sep 17 00:00:00 2001 From: Andrey Kurilin Date: Tue, 28 Aug 2018 15:51:21 +0300 Subject: [PATCH] Do not mute errors on check health of existing@openstack The current output of `rally env check` produces useless data which can say just Success/Failure had happened. To identify the root cause even debug mode doesn't help. This patch adds checks for exception classes and use different messages (not only 'bad user credentils') for different cases. Change-Id: I994017efdaac8e8894c660cdfd8e737dde04adf9 --- CHANGELOG.rst | 1 + rally_openstack/osclients.py | 62 ++++++++++-- rally_openstack/platforms/existing.py | 46 +++++---- tests/functional/test_cli_deployment.py | 13 +-- tests/unit/platforms/test_existing.py | 22 +++- tests/unit/test_osclients.py | 127 ++++++++++++++++++++---- tests/unit/test_workarounds.py | 5 + 7 files changed, 214 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cb261322..5121fef5 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -29,6 +29,7 @@ Changed * Our requirements are updated as like upper-constraints (the list of suggested tested versions to use) +* Error messages become more user-friendly in ``rally env check``. Removed ~~~~~~~ diff --git a/rally_openstack/osclients.py b/rally_openstack/osclients.py index 457a35c0..787336ed 100644 --- a/rally_openstack/osclients.py +++ b/rally_openstack/osclients.py @@ -31,6 +31,50 @@ LOG = logging.getLogger(__name__) CONF = cfg.CONF +class AuthenticationFailed(exceptions.AuthenticationFailed): + error_code = 220 + + msg_fmt = ("Failed to authenticate to %(url)s for user '%(username)s'" + " in project '%(project)s': %(message)s") + msg_fmt_2 = "%(message)s" + + def __init__(self, error, url, username, project): + kwargs = { + "error": error, + "url": url, + "username": username, + "project": project + } + self._helpful_trace = False + + from keystoneauth1 import exceptions as ks_exc + + if isinstance(error, ks_exc.ConnectionError): + # this type of errors is general for all users no need to include + # username, project name. The original error message should be + # self-sufficient + self.msg_fmt = self.msg_fmt_2 + message = error.message + if message.startswith("Unable to establish connection to"): + # this message contains too much info. + if "Max retries exceeded with url" in message: + if "HTTPConnectionPool" in message: + splitter = ": HTTPConnectionPool" + else: + splitter = ": HTTPSConnectionPool" + message = message.split(splitter, 1)[0] + elif isinstance(error, ks_exc.Unauthorized): + message = error.message.split(" (HTTP 401)", 1)[0] + else: + # something unexpected. include exception class as well. + self._helpful_trace = True + message = "[%s] %s" % (error.__class__.__name__, str(error)) + super(AuthenticationFailed, self).__init__(message=message, **kwargs) + + def is_trace_helpful(self): + return self._helpful_trace + + def configure(name, default_version=None, default_service_type=None, supported_versions=None): """OpenStack client class wrapper. @@ -230,19 +274,21 @@ class Keystone(OSClient): if "keystone_auth_ref" not in self.cache: sess, plugin = self.get_session() self.cache["keystone_auth_ref"] = plugin.get_access(sess) - except Exception as e: - if logging.is_debug(): + except Exception as original_e: + e = AuthenticationFailed( + error=original_e, + username=self.credential.username, + project=self.credential.tenant_name, + url=self.credential.auth_url + ) + if logging.is_debug() and e.is_trace_helpful(): LOG.exception("Unable to authenticate for user" " %(username)s in project" " %(tenant_name)s" % {"username": self.credential.username, "tenant_name": self.credential.tenant_name}) - raise exceptions.AuthenticationFailed( - username=self.credential.username, - project=self.credential.tenant_name, - url=self.credential.auth_url, - etype=e.__class__.__name__, - error=str(e)) + + raise e return self.cache["keystone_auth_ref"] def get_session(self, version=None): diff --git a/rally_openstack/platforms/existing.py b/rally_openstack/platforms/existing.py index 33714125..78f61ee4 100644 --- a/rally_openstack/platforms/existing.py +++ b/rally_openstack/platforms/existing.py @@ -144,32 +144,42 @@ class OpenStack(platform.Platform): def check_health(self): """Check whatever platform is alive.""" - if self.platform_data["admin"]: - try: - osclients.Clients( - self.platform_data["admin"]).verified_keystone() - except Exception: - d = copy.deepcopy(self.platform_data["admin"]) - d["password"] = "***" - return { - "available": False, - "message": ( - "Bad admin creds: \n%s" - % json.dumps(d, indent=2, sort_keys=True)), - "traceback": traceback.format_exc() - } - for user in self.platform_data["users"]: + users_to_check = self.platform_data["users"] + if self.platform_data["admin"]: + users_to_check.append(self.platform_data["admin"]) + + for user in users_to_check: try: - osclients.Clients(user).keystone() + if self.platform_data["admin"] == user: + osclients.Clients(user).verified_keystone() + else: + osclients.Clients(user).keystone() + except osclients.exceptions.RallyException as e: + # all rally native exceptions should provide user-friendly + # messages + return {"available": False, "message": e.format_message(), + # traceback is redundant here. Remove as soon as min + # required rally version will be updated + # More details here: + # https://review.openstack.org/597197 + "traceback": traceback.format_exc()} except Exception: d = copy.deepcopy(user) d["password"] = "***" + if logging.is_debug(): + LOG.exception("Something unexpected had happened while " + "validating OpenStack credentials.") + if self.platform_data["admin"] == user: + user_role = "admin" + else: + user_role = "user" return { "available": False, "message": ( - "Bad user creds: \n%s" - % json.dumps(d, indent=2, sort_keys=True)), + "Bad %s creds: \n%s" + % (user_role, + json.dumps(d, indent=2, sort_keys=True))), "traceback": traceback.format_exc() } diff --git a/tests/functional/test_cli_deployment.py b/tests/functional/test_cli_deployment.py index 68bc03b4..2aec84b0 100644 --- a/tests/functional/test_cli_deployment.py +++ b/tests/functional/test_cli_deployment.py @@ -86,17 +86,8 @@ class DeploymentTestCase(unittest.TestCase): rally("--debug deployment check") except utils.RallyCliError as e: self.assertIn( - "[-] Unable to authenticate for user %(username)s in" - " project %(tenant_name)s" % - {"username": TEST_ENV["OS_USERNAME"], - "tenant_name": TEST_ENV["OS_TENANT_NAME"]}, - str(e)) - self.assertIn( - "AuthenticationFailed: Failed to authenticate to %(auth_url)s" - " for user '%(username)s' in project '%(tenant_name)s'" % - {"auth_url": TEST_ENV["OS_AUTH_URL"], - "username": TEST_ENV["OS_USERNAME"], - "tenant_name": TEST_ENV["OS_TENANT_NAME"]}, + "AuthenticationFailed: Unable to establish connection to " + "%s" % TEST_ENV["OS_AUTH_URL"], str(e)) else: self.fail("rally deployment fails to raise error for wrong" diff --git a/tests/unit/platforms/test_existing.py b/tests/unit/platforms/test_existing.py index 752341f2..c71773cf 100644 --- a/tests/unit/platforms/test_existing.py +++ b/tests/unit/platforms/test_existing.py @@ -18,6 +18,7 @@ import jsonschema import mock from rally.env import env_mgr from rally.env import platform +from rally import exceptions from rally_openstack.platforms import existing from tests.unit import test @@ -245,14 +246,27 @@ class ExistingPlatformTestCase(PlatformBaseTestCase): self._check_health_schema(result) self.assertEqual({"available": True}, result) mock_clients.assert_has_calls( - [mock.call(pdata["admin"]), mock.call().verified_keystone(), - mock.call(pdata["users"][0]), mock.call().keystone(), - mock.call(pdata["users"][1]), mock.call().keystone()]) + [mock.call(pdata["users"][0]), mock.call().keystone(), + mock.call(pdata["users"][1]), mock.call().keystone(), + mock.call(pdata["admin"]), mock.call().verified_keystone()]) + + @mock.patch("rally_openstack.osclients.Clients") + def test_check_failed_with_native_rally_exc(self, mock_clients): + e = exceptions.RallyException("foo") + mock_clients.return_value.keystone.side_effect = e + pdata = {"admin": None, + "users": [{"username": "balbab", "password": "12345"}]} + result = existing.OpenStack({}, platform_data=pdata).check_health() + self._check_health_schema(result) + self.assertEqual({"available": False, "message": e.format_message(), + "traceback": mock.ANY}, + result) @mock.patch("rally_openstack.osclients.Clients") def test_check_failed_admin(self, mock_clients): mock_clients.return_value.verified_keystone.side_effect = Exception - pdata = {"admin": {"username": "balbab", "password": "12345"}} + pdata = {"admin": {"username": "balbab", "password": "12345"}, + "users": []} result = existing.OpenStack({}, platform_data=pdata).check_health() self._check_health_schema(result) self.assertEqual( diff --git a/tests/unit/test_osclients.py b/tests/unit/test_osclients.py index 4e2d78c2..1333a777 100644 --- a/tests/unit/test_osclients.py +++ b/tests/unit/test_osclients.py @@ -283,38 +283,123 @@ class TestCreateKeystoneClient(test.TestCase, OSClientTestCaseUtils): keystone.auth_ref mock_keystone_get_session.assert_called_once_with() - @mock.patch("keystoneauth1.identity.base.BaseIdentityPlugin.get_access") - def test_auth_ref_fails(self, mock_get_access): - mock_get_access.side_effect = Exception + @mock.patch("%s.LOG.exception" % PATH) + @mock.patch("%s.logging.is_debug" % PATH) + def test_auth_ref_fails(self, mock_is_debug, mock_log_exception): + mock_is_debug.return_value = False keystone = osclients.Keystone(self.credential, {}, {}) + session = mock.Mock() + auth_plugin = mock.Mock() + auth_plugin.get_access.side_effect = Exception + keystone.get_session = mock.Mock(return_value=(session, auth_plugin)) - try: - keystone.auth_ref - except exceptions.AuthenticationFailed: - pass - else: - self.fail("keystone.auth_ref didn't raise" - " exceptions.AuthenticationFailed") + self.assertRaises(osclients.AuthenticationFailed, + lambda: keystone.auth_ref) + + self.assertFalse(mock_log_exception.called) + mock_is_debug.assert_called_once_with() + auth_plugin.get_access.assert_called_once_with(session) @mock.patch("%s.LOG.exception" % PATH) @mock.patch("%s.logging.is_debug" % PATH) - @mock.patch("keystoneauth1.identity.base.BaseIdentityPlugin.get_access") - def test_auth_ref_debug(self, mock_get_access, - mock_is_debug, mock_log_exception): + def test_auth_ref_fails_debug(self, mock_is_debug, mock_log_exception): mock_is_debug.return_value = True - mock_get_access.side_effect = Exception keystone = osclients.Keystone(self.credential, {}, {}) + session = mock.Mock() + auth_plugin = mock.Mock() + auth_plugin.get_access.side_effect = Exception + keystone.get_session = mock.Mock(return_value=(session, auth_plugin)) - try: - keystone.auth_ref - except exceptions.AuthenticationFailed: - pass - else: - self.fail("keystone.auth_ref didn't raise" - " exceptions.AuthenticationFailed") + self.assertRaises(osclients.AuthenticationFailed, + lambda: keystone.auth_ref) mock_log_exception.assert_called_once_with(mock.ANY) mock_is_debug.assert_called_once_with() + auth_plugin.get_access.assert_called_once_with(session) + + @mock.patch("%s.LOG.exception" % PATH) + @mock.patch("%s.logging.is_debug" % PATH) + def test_auth_ref_fails_debug_with_native_keystone_error( + self, mock_is_debug, mock_log_exception): + from keystoneauth1 import exceptions as ks_exc + + mock_is_debug.return_value = True + keystone = osclients.Keystone(self.credential, {}, {}) + session = mock.Mock() + auth_plugin = mock.Mock() + auth_plugin.get_access.side_effect = ks_exc.ConnectFailure("foo") + keystone.get_session = mock.Mock(return_value=(session, auth_plugin)) + + self.assertRaises(osclients.AuthenticationFailed, + lambda: keystone.auth_ref) + + self.assertFalse(mock_log_exception.called) + mock_is_debug.assert_called_once_with() + auth_plugin.get_access.assert_called_once_with(session) + + def test_authentication_failed_exception(self): + from keystoneauth1 import exceptions as ks_exc + + original_e = KeyError("Oops") + e = osclients.AuthenticationFailed( + url="https://example.com", username="foo", project="project", + error=original_e + ) + self.assertEqual( + "Failed to authenticate to https://example.com for user 'foo' in " + "project 'project': [KeyError] 'Oops'", + e.format_message()) + + original_e = ks_exc.Unauthorized("The request you have made requires " + "authentication.", request_id="ID") + e = osclients.AuthenticationFailed( + url="https://example.com", username="foo", project="project", + error=original_e + ) + self.assertEqual( + "Failed to authenticate to https://example.com for user 'foo' in " + "project 'project': The request you have made requires " + "authentication.", + e.format_message()) + + original_e = ks_exc.ConnectionError("Some user-friendly native error") + e = osclients.AuthenticationFailed( + url="https://example.com", username="foo", project="project", + error=original_e + ) + self.assertEqual("Some user-friendly native error", + e.format_message()) + + original_e = ks_exc.ConnectionError( + "Unable to establish connection to https://example.com:500: " + "HTTPSConnectionPool(host='example.com', port=500): Max retries " + "exceeded with url: / (Caused by NewConnectionError(': " + "Failed to establish a new connection: [Errno 101] Network " + "is unreachable") + e = osclients.AuthenticationFailed( + url="https://example.com", username="foo", project="project", + error=original_e + ) + self.assertEqual( + "Unable to establish connection to https://example.com:500", + e.format_message()) + + original_e = ks_exc.ConnectionError( + "Unable to establish connection to https://example.com:500: " + # another pool class + "HTTPConnectionPool(host='example.com', port=500): Max retries " + "exceeded with url: / (Caused by NewConnectionError(': " + "Failed to establish a new connection: [Errno 101] Network " + "is unreachable") + e = osclients.AuthenticationFailed( + url="https://example.com", username="foo", project="project", + error=original_e + ) + self.assertEqual( + "Unable to establish connection to https://example.com:500", + e.format_message()) @ddt.ddt diff --git a/tests/unit/test_workarounds.py b/tests/unit/test_workarounds.py index b57a867e..77ac2f87 100644 --- a/tests/unit/test_workarounds.py +++ b/tests/unit/test_workarounds.py @@ -38,6 +38,11 @@ class WorkaroundTestCase(test.TestCase): "'rally_openstack.validators' module has a check to do not " "register 'required_platforms@openstack' validator for old Rally " "releases." + ]), + ([1, 2], [ + "'existing@openstack' platform puts 'traceback' in check method " + "in case of native keystone errors. It is redundant. " + "See https://review.openstack.org/597197" ]) ]