From 2f84e820fdd9bbc55c9ea1a4083becad82a754f3 Mon Sep 17 00:00:00 2001 From: Kamil Braun Date: Wed, 18 Jan 2023 16:50:26 +0100 Subject: [PATCH] test/pylib: scylla_cluster: return error details from test framework endpoints If an endpoint handler throws an exception, the details of the exception are not returned to the client. Normally this is desirable so that information is not leaked, but in this test framework we do want to return the details to the client so it can log a useful error message. Do it by wrapping every handler into a catch clause that returns the exception message. Also modify a bit how HTTPErrors are rendered so it's easier to discern the actual body of the error from other details (such as the params used to make the request etc.) Before: ``` E test.pylib.rest_client.HTTPError: HTTP error 500: 500 Internal Server Error E E Server got itself in trouble, params None, json None, uri http+unix://api/cluster/before-test/test_stuff ``` After: ``` E test.pylib.rest_client.HTTPError: HTTP error 500, uri: http+unix://api/cluster/before-test/test_stuff, params: None, json: None, body: E Failed to start server at host 127.155.129.1. E Check the log files: E /home/kbraun/dev/scylladb/testlog/test.py.dev.log E /home/kbraun/dev/scylladb/testlog/dev/scylla-1.log ``` Closes #12563 --- test/pylib/rest_client.py | 9 ++++-- test/pylib/scylla_cluster.py | 61 +++++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/test/pylib/rest_client.py b/test/pylib/rest_client.py index db2a521690..c2ea2a5133 100644 --- a/test/pylib/rest_client.py +++ b/test/pylib/rest_client.py @@ -21,14 +21,17 @@ logger = logging.getLogger(__name__) class HTTPError(Exception): - def __init__(self, uri, code, message): + def __init__(self, uri, code, params, json, message): super().__init__(message) self.uri = uri self.code = code + self.params = params + self.json = json self.message = message def __str__(self): - return f"HTTP error {self.code}: {self.message}, uri {self.uri}" + return f"HTTP error {self.code}, uri: {self.uri}, " \ + f"params: {self.params}, json: {self.json}, body:\n{self.message}" # TODO: support ssl and verify_ssl @@ -63,7 +66,7 @@ class RESTClient(metaclass=ABCMeta): params = params, json = json, timeout = client_timeout) as resp: if resp.status != 200: text = await resp.text() - raise HTTPError(uri, resp.status, f"{text}, params {params}, json {json}") + raise HTTPError(uri, resp.status, params, json, text) if response_type is not None: # Return response.text() or response.json() return await getattr(resp, response_type)() diff --git a/test/pylib/scylla_cluster.py b/test/pylib/scylla_cluster.py index 5cf7aff08b..e12ec24688 100644 --- a/test/pylib/scylla_cluster.py +++ b/test/pylib/scylla_cluster.py @@ -17,6 +17,7 @@ import pathlib import shutil import tempfile import time +import traceback from typing import Optional, Dict, List, Set, Tuple, Callable, AsyncIterator, NamedTuple, Union import uuid from io import BufferedWriter @@ -884,27 +885,45 @@ class ScyllaClusterManager: def _setup_routes(self, app: aiohttp.web.Application) -> None: - app.router.add_get('/up', self._manager_up) - app.router.add_get('/cluster/up', self._cluster_up) - app.router.add_get('/cluster/is-dirty', self._is_dirty) - app.router.add_get('/cluster/replicas', self._cluster_replicas) - app.router.add_get('/cluster/running-servers', self._cluster_running_servers) - app.router.add_get('/cluster/host-ip/{server_id}', self._cluster_server_ip_addr) - app.router.add_get('/cluster/host-id/{server_id}', self._cluster_host_id) - app.router.add_get('/cluster/before-test/{test_case_name}', self._before_test_req) - app.router.add_get('/cluster/after-test', self._after_test) - app.router.add_get('/cluster/mark-dirty', self._mark_dirty) - app.router.add_get('/cluster/server/{server_id}/stop', self._cluster_server_stop) - app.router.add_get('/cluster/server/{server_id}/stop_gracefully', - self._cluster_server_stop_gracefully) - app.router.add_get('/cluster/server/{server_id}/start', self._cluster_server_start) - app.router.add_get('/cluster/server/{server_id}/restart', self._cluster_server_restart) - app.router.add_put('/cluster/addserver', self._cluster_server_add) - app.router.add_put('/cluster/remove-node/{initiator}', self._cluster_remove_node) - app.router.add_get('/cluster/decommission-node/{server_id}', - self._cluster_decommission_node) - app.router.add_get('/cluster/server/{server_id}/get_config', self._server_get_config) - app.router.add_put('/cluster/server/{server_id}/update_config', self._server_update_config) + def make_catching_handler(handler: Callable) -> Callable: + async def catching_handler(request) -> aiohttp.web.Response: + """Catch all exceptions and return them to the client. + Without this, the client would get an 'Internal server error' message + without any details. Thanks to this the test log shows the actual error. + """ + try: + return await handler(request) + except Exception as e: + tb = traceback.format_exc() + self.logger.error(f'Exception when executing {handler.__name__}: {e}\n{tb}') + return aiohttp.web.Response(status=500, text=str(e)) + return catching_handler + + def add_get(route: str, handler: Callable): + app.router.add_get(route, make_catching_handler(handler)) + + def add_put(route: str, handler: Callable): + app.router.add_put(route, make_catching_handler(handler)) + + add_get('/up', self._manager_up) + add_get('/cluster/up', self._cluster_up) + add_get('/cluster/is-dirty', self._is_dirty) + add_get('/cluster/replicas', self._cluster_replicas) + add_get('/cluster/running-servers', self._cluster_running_servers) + add_get('/cluster/host-ip/{server_id}', self._cluster_server_ip_addr) + add_get('/cluster/host-id/{server_id}', self._cluster_host_id) + add_get('/cluster/before-test/{test_case_name}', self._before_test_req) + add_get('/cluster/after-test', self._after_test) + add_get('/cluster/mark-dirty', self._mark_dirty) + add_get('/cluster/server/{server_id}/stop', self._cluster_server_stop) + add_get('/cluster/server/{server_id}/stop_gracefully', self._cluster_server_stop_gracefully) + add_get('/cluster/server/{server_id}/start', self._cluster_server_start) + add_get('/cluster/server/{server_id}/restart', self._cluster_server_restart) + add_put('/cluster/addserver', self._cluster_server_add) + add_put('/cluster/remove-node/{initiator}', self._cluster_remove_node) + add_get('/cluster/decommission-node/{server_id}', self._cluster_decommission_node) + add_get('/cluster/server/{server_id}/get_config', self._server_get_config) + add_put('/cluster/server/{server_id}/update_config', self._server_update_config) async def _manager_up(self, _request) -> aiohttp.web.Response: return aiohttp.web.Response(text=f"{self.is_running}")