Expand handling of blocked EP registration

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``.

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:

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")

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(

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,15 +193,26 @@ 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,
     mock_conf,
     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