From d224e4a6952d5fb8e6a95a47efd7a6bd47d44a71 Mon Sep 17 00:00:00 2001 From: Yingge He <157551214+yinggeh@users.noreply.github.com> Date: Wed, 10 Apr 2024 17:47:07 -0700 Subject: [PATCH] Disable Dynamic Log File (#7092) * Deprecate dynamic log file * Update error message --- qa/L0_logging/logging_endpoint_test.py | 74 ++++++++++++++------------ qa/L0_logging/test.sh | 9 +++- src/grpc/grpc_server.cc | 30 ++--------- src/http_server.cc | 25 ++------- 4 files changed, 56 insertions(+), 82 deletions(-) diff --git a/qa/L0_logging/logging_endpoint_test.py b/qa/L0_logging/logging_endpoint_test.py index 26f98de3da..106afba6f1 100755 --- a/qa/L0_logging/logging_endpoint_test.py +++ b/qa/L0_logging/logging_endpoint_test.py @@ -38,6 +38,7 @@ import tritonclient.grpc as grpcclient import tritonclient.http as httpclient from google.protobuf import json_format +from tritonclient.utils import InferenceServerException # Similar set up as dynamic batcher tests @@ -49,7 +50,6 @@ def tearDown(self): # tearDown() is properly executed and not affecting start state of # other test cases clear_settings = { - "log_file": "", "log_info": True, "log_warning": True, "log_error": True, @@ -126,7 +126,7 @@ def test_http_update_settings(self): # by the server self.check_server_initial_state() - expected_log_settings_1 = { + log_settings_1 = { "log_file": "log_file.log", "log_info": True, "log_warning": True, @@ -134,76 +134,88 @@ def test_http_update_settings(self): "log_verbose_level": 0, "log_format": "default", } - expected_log_settings_2 = { - "log_file": "log_file.log", + expected_log_settings_1 = { + "error": "log file location can not be updated through network protocol" + } + + log_settings_2 = { "log_info": False, "log_warning": True, "log_error": True, "log_verbose_level": 0, "log_format": "default", } - expected_log_settings_3 = { - "log_file": "log_file.log", + expected_log_settings_2 = log_settings_2.copy() + expected_log_settings_2["log_file"] = "" + + log_settings_3 = { "log_info": False, "log_warning": False, "log_error": True, "log_verbose_level": 0, "log_format": "default", } - expected_log_settings_4 = { - "log_file": "log_file.log", + expected_log_settings_3 = log_settings_3.copy() + expected_log_settings_3["log_file"] = "" + + log_settings_4 = { "log_info": False, "log_warning": False, "log_error": False, "log_verbose_level": 0, "log_format": "default", } - expected_log_settings_5 = { - "log_file": "log_file.log", + expected_log_settings_4 = log_settings_4.copy() + expected_log_settings_4["log_file"] = "" + + log_settings_5 = { "log_info": False, "log_warning": False, "log_error": False, "log_verbose_level": 1, "log_format": "default", } - expected_log_settings_6 = { - "log_file": "log_file.log", + expected_log_settings_5 = log_settings_5.copy() + expected_log_settings_5["log_file"] = "" + + log_settings_6 = { "log_info": False, "log_warning": False, "log_error": False, "log_verbose_level": 1, "log_format": "ISO8601", } + expected_log_settings_6 = log_settings_6.copy() + expected_log_settings_6["log_file"] = "" triton_client = httpclient.InferenceServerClient("localhost:8000") - self.assertEqual( - expected_log_settings_1, - triton_client.update_log_settings(settings=expected_log_settings_1), - "Unexpected updated log settings", - ) + with self.assertRaisesRegex( + InferenceServerException, expected_log_settings_1["error"] + ) as e: + triton_client.update_log_settings(settings=log_settings_1) self.assertEqual( expected_log_settings_2, - triton_client.update_log_settings(settings=expected_log_settings_2), + triton_client.update_log_settings(settings=log_settings_2), "Unexpected updated log settings", ) self.assertEqual( expected_log_settings_3, - triton_client.update_log_settings(settings=expected_log_settings_3), + triton_client.update_log_settings(settings=log_settings_3), "Unexpected updated log settings", ) self.assertEqual( expected_log_settings_4, - triton_client.update_log_settings(settings=expected_log_settings_4), + triton_client.update_log_settings(settings=log_settings_4), "Unexpected updated log settings", ) self.assertEqual( expected_log_settings_5, - triton_client.update_log_settings(settings=expected_log_settings_5), + triton_client.update_log_settings(settings=log_settings_5), "Unexpected updated log settings", ) self.assertEqual( expected_log_settings_6, - triton_client.update_log_settings(settings=expected_log_settings_6), + triton_client.update_log_settings(settings=log_settings_6), "Unexpected updated log settings", ) @@ -215,7 +227,6 @@ def test_grpc_update_settings(self): triton_client = grpcclient.InferenceServerClient("localhost:8001") log_settings_1 = { - "log_file": "log_file.log", "log_info": True, "log_warning": True, "log_error": True, @@ -227,7 +238,7 @@ def test_grpc_update_settings(self): json.dumps( { "settings": { - "log_file": {"stringParam": "log_file.log"}, + "log_file": {"stringParam": ""}, "log_info": {"boolParam": True}, "log_warning": {"boolParam": True}, "log_error": {"boolParam": True}, @@ -246,7 +257,6 @@ def test_grpc_update_settings(self): ) log_settings_2 = { - "log_file": "log_file.log", "log_info": False, "log_warning": True, "log_error": True, @@ -258,7 +268,7 @@ def test_grpc_update_settings(self): json.dumps( { "settings": { - "log_file": {"stringParam": "log_file.log"}, + "log_file": {"stringParam": ""}, "log_info": {"boolParam": False}, "log_warning": {"boolParam": True}, "log_error": {"boolParam": True}, @@ -277,7 +287,6 @@ def test_grpc_update_settings(self): ) log_settings_3 = { - "log_file": "log_file.log", "log_info": False, "log_warning": False, "log_error": True, @@ -289,7 +298,7 @@ def test_grpc_update_settings(self): json.dumps( { "settings": { - "log_file": {"stringParam": "log_file.log"}, + "log_file": {"stringParam": ""}, "log_info": {"boolParam": False}, "log_warning": {"boolParam": False}, "log_error": {"boolParam": True}, @@ -308,7 +317,6 @@ def test_grpc_update_settings(self): ) log_settings_4 = { - "log_file": "log_file.log", "log_info": False, "log_warning": False, "log_error": False, @@ -320,7 +328,7 @@ def test_grpc_update_settings(self): json.dumps( { "settings": { - "log_file": {"stringParam": "log_file.log"}, + "log_file": {"stringParam": ""}, "log_info": {"boolParam": False}, "log_warning": {"boolParam": False}, "log_error": {"boolParam": False}, @@ -339,7 +347,6 @@ def test_grpc_update_settings(self): ) log_settings_5 = { - "log_file": "log_file.log", "log_info": False, "log_warning": False, "log_error": False, @@ -351,7 +358,7 @@ def test_grpc_update_settings(self): json.dumps( { "settings": { - "log_file": {"stringParam": "log_file.log"}, + "log_file": {"stringParam": ""}, "log_info": {"boolParam": False}, "log_warning": {"boolParam": False}, "log_error": {"boolParam": False}, @@ -370,7 +377,6 @@ def test_grpc_update_settings(self): ) log_settings_6 = { - "log_file": "log_file.log", "log_info": False, "log_warning": False, "log_error": False, @@ -382,7 +388,7 @@ def test_grpc_update_settings(self): json.dumps( { "settings": { - "log_file": {"stringParam": "log_file.log"}, + "log_file": {"stringParam": ""}, "log_info": {"boolParam": False}, "log_warning": {"boolParam": False}, "log_error": {"boolParam": False}, diff --git a/qa/L0_logging/test.sh b/qa/L0_logging/test.sh index 160bffe3dd..e5012d5a80 100755 --- a/qa/L0_logging/test.sh +++ b/qa/L0_logging/test.sh @@ -202,7 +202,12 @@ rm -f ./curl.out code=`curl -s -w %{http_code} -o ./curl.out -d'{"log_file":"other_log.log"}' localhost:8000/v2/logging` set +e -verify_correct_settings "other_log.log" "true" "true" "true" "1" "default" +# updating log file location no longer supported +if [ `grep -c "\"error\":\"log file location can not be updated through network protocol\"" ./curl.out` != "1" ]; then + echo -e "\n***\n*** Test Failed: Incorrect Error Response\n***" + RET=1 +fi +verify_correct_settings "log_file.log" "true" "true" "true" "1" "default" $SIMPLE_HTTP_CLIENT >> client_test_log_file.log 2>&1 if [ $? -ne 0 ]; then @@ -225,7 +230,7 @@ if [ $actual_log_count -lt $expected_log_count ]; then RET=1 fi expected_other_log_count=31 -actual_other_log_count=$(grep -c ^[IWEV][0-9][0-9][0-9][0-9].* ./other_log.log) +actual_other_log_count=$(grep -c ^[IWEV][0-9][0-9][0-9][0-9].* ./log_file.log) if [ $actual_other_log_count -lt $expected_other_log_count ]; then echo $actual_other_log_count echo $expected_other_log_count diff --git a/src/grpc/grpc_server.cc b/src/grpc/grpc_server.cc index 63e2854208..f208658e51 100644 --- a/src/grpc/grpc_server.cc +++ b/src/grpc/grpc_server.cc @@ -1463,32 +1463,10 @@ CommonHandler::RegisterLogging() static std::string setting_name = "log_file"; auto it = request.settings().find(setting_name); if (it != request.settings().end()) { - const auto& log_param = it->second; - if (log_param.parameter_choice_case() != - inference::LogSettingsRequest_SettingValue::ParameterChoiceCase:: - kStringParam) { - err = TRITONSERVER_ErrorNew( - TRITONSERVER_ERROR_INVALID_ARG, - (std::string("expect string for '") + setting_name + "'") - .c_str()); - GOTO_IF_ERR(err, earlyexit); - } else { - // Set new settings in server then in core - const std::string& log_file_path = it->second.string_param(); - const std::string& error = LOG_SET_OUT_FILE(log_file_path); - if (!error.empty()) { - err = TRITONSERVER_ErrorNew( - TRITONSERVER_ERROR_INTERNAL, (error).c_str()); - GOTO_IF_ERR(err, earlyexit); - } - // Okay to pass nullptr because we know the update will be applied - // to the global object. - err = TRITONSERVER_ServerOptionsSetLogFile( - nullptr, log_file_path.c_str()); - if (err != nullptr) { - GOTO_IF_ERR(err, earlyexit); - } - } + err = TRITONSERVER_ErrorNew( + TRITONSERVER_ERROR_UNSUPPORTED, + "log file location can not be updated through network protocol"); + GOTO_IF_ERR(err, earlyexit); } } { diff --git a/src/http_server.cc b/src/http_server.cc index 3d9806c8da..44539ef3bc 100644 --- a/src/http_server.cc +++ b/src/http_server.cc @@ -2080,7 +2080,6 @@ HTTPAPIServer::HandleLogging(evhtp_request_t* req) "unexpected error getting dynamic logging request buffers")); } } - TRITONSERVER_Error* err = nullptr; triton::common::TritonJson::Value request; size_t buffer_len = evbuffer_get_length(req->buffer_in); RETURN_AND_RESPOND_IF_ERR( @@ -2090,25 +2089,11 @@ HTTPAPIServer::HandleLogging(evhtp_request_t* req) triton::common::TritonJson::Value setting_json; if (request.Find("log_file", &setting_json)) { if (!setting_json.IsNull()) { - // Set new settings in server then in core - std::string log_file_path; - RETURN_AND_RESPOND_IF_ERR(req, setting_json.AsString(&log_file_path)); - const std::string& error = LOG_SET_OUT_FILE(log_file_path); - if (!error.empty()) { - RETURN_AND_RESPOND_IF_ERR( - req, TRITONSERVER_ErrorNew( - TRITONSERVER_ERROR_UNAVAILABLE, (error).c_str())); - } - // Okay to pass nullptr because we know the update will be applied - // to the global object. - err = TRITONSERVER_ServerOptionsSetLogFile( - nullptr, log_file_path.c_str()); - if (err != nullptr) { - RETURN_AND_RESPOND_IF_ERR( - req, TRITONSERVER_ErrorNew( - TRITONSERVER_ERROR_UNAVAILABLE, - (TRITONSERVER_ErrorMessage(err)))); - } + RETURN_AND_RESPOND_IF_ERR( + req, TRITONSERVER_ErrorNew( + TRITONSERVER_ERROR_UNSUPPORTED, + "log file location can not be updated through network " + "protocol")); } } if (request.Find("log_info", &setting_json)) {