From 039d2a3862494f86d4c35c6761b32f0878e26bc7 Mon Sep 17 00:00:00 2001 From: Reid Mello <30907815+rjmello@users.noreply.github.com> Date: Mon, 23 Oct 2023 12:45:33 -0400 Subject: [PATCH] Expand handling of blocked EP registration --- ..._121708_30907815+rjmello_ep_reg_errors.rst | 5 ++ .../endpoint/endpoint.py | 19 ++++++-- .../endpoint/endpoint_manager.py | 19 ++++++-- .../tests/unit/test_endpoint_unit.py | 47 ++++++++++++++----- .../tests/unit/test_endpointmanager_unit.py | 39 +++++++++++---- 5 files changed, 98 insertions(+), 31 deletions(-) create mode 100644 changelog.d/20231023_121708_30907815+rjmello_ep_reg_errors.rst diff --git a/changelog.d/20231023_121708_30907815+rjmello_ep_reg_errors.rst b/changelog.d/20231023_121708_30907815+rjmello_ep_reg_errors.rst new file mode 100644 index 000000000..6fb26ba04 --- /dev/null +++ b/changelog.d/20231023_121708_30907815+rjmello_ep_reg_errors.rst @@ -0,0 +1,5 @@ +New Functionality +^^^^^^^^^^^^^^^^^ + +- Expand cases in which we return a meaningful exit code and message after endpoint + registration failures when calling ``globus-compute-endpoint start``. diff --git a/compute_endpoint/globus_compute_endpoint/endpoint/endpoint.py b/compute_endpoint/globus_compute_endpoint/endpoint/endpoint.py index a3c07987f..275e62721 100644 --- a/compute_endpoint/globus_compute_endpoint/endpoint/endpoint.py +++ b/compute_endpoint/globus_compute_endpoint/endpoint/endpoint.py @@ -13,6 +13,7 @@ import sys import typing as t import uuid +from http import HTTPStatus import daemon import daemon.pidfile @@ -390,12 +391,20 @@ def start_endpoint( ) except GlobusAPIError as e: - if e.http_status in (409, 410, 423): - # CONFLICT, GONE or LOCKED - blocked_msg = f"Endpoint registration blocked. [{e.text}]" - print(blocked_msg) - log.warning(blocked_msg) + blocked_msg = f"Endpoint registration blocked. [{e.text}]" + log.warning(blocked_msg) + print(blocked_msg) + if e.http_status in ( + HTTPStatus.CONFLICT, + HTTPStatus.LOCKED, + HTTPStatus.NOT_FOUND, + ): exit(os.EX_UNAVAILABLE) + elif e.http_status in ( + HTTPStatus.BAD_REQUEST, + HTTPStatus.UNPROCESSABLE_ENTITY, + ): + exit(os.EX_DATAERR) raise except NetworkError as e: diff --git a/compute_endpoint/globus_compute_endpoint/endpoint/endpoint_manager.py b/compute_endpoint/globus_compute_endpoint/endpoint/endpoint_manager.py index 6b3726e7a..3ac23b256 100644 --- a/compute_endpoint/globus_compute_endpoint/endpoint/endpoint_manager.py +++ b/compute_endpoint/globus_compute_endpoint/endpoint/endpoint_manager.py @@ -16,6 +16,7 @@ import time import typing as t from datetime import datetime +from http import HTTPStatus import globus_compute_sdk as GC from cachetools import TTLCache @@ -129,12 +130,20 @@ def __init__( assert reg_info is not None, "Empty response from Compute API" except GlobusAPIError as e: - if e.http_status == 409 or e.http_status == 423: - # RESOURCE_CONFLICT or RESOURCE_LOCKED - blocked_msg = f"Endpoint registration blocked. [{e.text}]" - print(blocked_msg) - log.warning(blocked_msg) + blocked_msg = f"Endpoint registration blocked. [{e.text}]" + log.warning(blocked_msg) + print(blocked_msg) + if e.http_status in ( + HTTPStatus.CONFLICT, + HTTPStatus.LOCKED, + HTTPStatus.NOT_FOUND, + ): exit(os.EX_UNAVAILABLE) + elif e.http_status in ( + HTTPStatus.BAD_REQUEST, + HTTPStatus.UNPROCESSABLE_ENTITY, + ): + exit(os.EX_DATAERR) raise except NetworkError as e: log.exception("Network error while registering multi-user endpoint") diff --git a/compute_endpoint/tests/unit/test_endpoint_unit.py b/compute_endpoint/tests/unit/test_endpoint_unit.py index 3bc149d25..40e8dabaf 100644 --- a/compute_endpoint/tests/unit/test_endpoint_unit.py +++ b/compute_endpoint/tests/unit/test_endpoint_unit.py @@ -9,6 +9,7 @@ import uuid from collections import namedtuple from contextlib import redirect_stdout +from http import HTTPStatus from types import SimpleNamespace from unittest import mock @@ -24,7 +25,7 @@ serialize_config, ) from globus_compute_endpoint.endpoint.endpoint import Endpoint -from globus_sdk import NetworkError +from globus_sdk import GlobusAPIError, NetworkError from pytest_mock import MockFixture _mock_base = "globus_compute_endpoint.endpoint.endpoint." @@ -263,39 +264,63 @@ def test_register_endpoint_invalid_response( assert other_endpoint_id in mock_log.error.call_args[0][0] -@pytest.mark.parametrize("ret_value", [[409, "Conflict"], [423, "Locked"]]) +@pytest.mark.parametrize( + "exit_code,status_code", + ( + (os.EX_UNAVAILABLE, HTTPStatus.CONFLICT), + (os.EX_UNAVAILABLE, HTTPStatus.LOCKED), + (os.EX_UNAVAILABLE, HTTPStatus.NOT_FOUND), + (os.EX_DATAERR, HTTPStatus.BAD_REQUEST), + (os.EX_DATAERR, HTTPStatus.UNPROCESSABLE_ENTITY), + ("Error", 418), # IM_A_TEAPOT + ), +) @responses.activate -def test_register_endpoint_locked_conflict_print( +def test_register_endpoint_blocked( mocker, fs, register_endpoint_failure_response, get_standard_compute_client, mock_ep_data, - ret_value, + randomstring, + exit_code, + status_code, ): """ Check to ensure endpoint registration escalates up with API error """ - ret_code, ret_text = ret_value + mock_log = mocker.patch(f"{_mock_base}log") mock_gcc = get_standard_compute_client() mocker.patch(f"{_mock_base}Endpoint.get_funcx_client").return_value = mock_gcc f = io.StringIO() ep, ep_dir, log_to_console, no_color, ep_conf = mock_ep_data ep_id = str(uuid.uuid4()) + some_err = randomstring() register_endpoint_failure_response( endpoint_id=ep_id, - status_code=ret_code, - msg=ret_text, + status_code=status_code, + msg=some_err, ) + with redirect_stdout(f): - with pytest.raises(SystemExit) as pytest_exc: + with pytest.raises((GlobusAPIError, SystemExit)) as pytexc: ep.start_endpoint( ep_dir, ep_id, ep_conf, log_to_console, no_color, reg_info={} ) - err_msg = f.getvalue() - assert "Endpoint registration blocked" in err_msg and ret_text in err_msg - assert pytest_exc.value.code == os.EX_UNAVAILABLE + stdout_msg = f.getvalue() + + assert mock_log.warning.called + a, *_ = mock_log.warning.call_args + assert some_err in str(a), "Expected upstream response still shared" + + assert some_err in stdout_msg, f"Expecting error message in stdout ({stdout_msg})" + assert pytexc.value.code == exit_code, "Expecting meaningful exit code" + + if exit_code == "Error": + # The other route tests SystemExit; nominally this route is an unhandled + # traceback -- good. We should _not_ blanket hide all exceptions. + assert pytexc.value.http_status == status_code def test_register_endpoint_already_active( diff --git a/compute_endpoint/tests/unit/test_endpointmanager_unit.py b/compute_endpoint/tests/unit/test_endpointmanager_unit.py index 42a1eed6e..c91aa88fc 100644 --- a/compute_endpoint/tests/unit/test_endpointmanager_unit.py +++ b/compute_endpoint/tests/unit/test_endpointmanager_unit.py @@ -1,5 +1,6 @@ import fcntl import getpass +import io import json import os import pathlib @@ -15,6 +16,8 @@ import typing as t import uuid from collections import namedtuple +from contextlib import redirect_stdout +from http import HTTPStatus from unittest import mock import jinja2 @@ -190,8 +193,18 @@ def test_sets_process_title( @responses.activate -@pytest.mark.parametrize("status_code", [409, 423, 418]) -def test_gracefully_exits_if_in_conflict_or_locked( +@pytest.mark.parametrize( + "exit_code,status_code", + ( + (os.EX_UNAVAILABLE, HTTPStatus.CONFLICT), + (os.EX_UNAVAILABLE, HTTPStatus.LOCKED), + (os.EX_UNAVAILABLE, HTTPStatus.NOT_FOUND), + (os.EX_DATAERR, HTTPStatus.BAD_REQUEST), + (os.EX_DATAERR, HTTPStatus.UNPROCESSABLE_ENTITY), + ("Error", 418), # IM_A_TEAPOT + ), +) +def test_gracefully_exits_if_registration_blocked( mocker, register_endpoint_failure_response, conf_dir, @@ -199,6 +212,7 @@ def test_gracefully_exits_if_in_conflict_or_locked( endpoint_uuid, randomstring, get_standard_compute_client, + exit_code, status_code, ): mock_log = mocker.patch(f"{_MOCK_BASE}log") @@ -208,15 +222,20 @@ def test_gracefully_exits_if_in_conflict_or_locked( some_err = randomstring() register_endpoint_failure_response(status_code, some_err) - with pytest.raises((GlobusAPIError, SystemExit)) as pyexc: - EndpointManager(conf_dir, endpoint_uuid, mock_conf) + f = io.StringIO() + with redirect_stdout(f): + with pytest.raises((GlobusAPIError, SystemExit)) as pyexc: + EndpointManager(conf_dir, endpoint_uuid, mock_conf) + stdout_msg = f.getvalue() - if status_code in (409, 423): - assert pyexc.value.code == os.EX_UNAVAILABLE, "Expecting meaningful exit code" - assert mock_log.warning.called - a, *_ = mock_log.warning.call_args - assert some_err in str(a), "Expected upstream response still shared" - else: + assert mock_log.warning.called + a, *_ = mock_log.warning.call_args + assert some_err in str(a), "Expected upstream response still shared" + + assert some_err in stdout_msg, f"Expecting error message in stdout ({stdout_msg})" + assert pyexc.value.code == exit_code, "Expecting meaningful exit code" + + if exit_code == "Error": # The other route tests SystemExit; nominally this route is an unhandled # traceback -- good. We should _not_ blanket hide all exceptions. assert pyexc.value.http_status == status_code