From 21edad5b09a0b1b2ec32f074e49626e6e1c39d20 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 15 Jun 2021 19:48:48 +0000 Subject: [PATCH 1/2] Use proxy.config.log.hostname for rotated log filenames When ATS rotates logs it includes the hostname in the rotated log filename. The user has the ability to set the hostname via 'proxy.config.log.hostname', however ATS did not respected this configuration parameter when naming the rotated log file. Rather, ATS always used the result of gethostname(). This fixes the log rotation mechanism so that if the user sets 'proxy.config.log.hostname' to a non-default value, then ATS will use that when naming the rotated log filename. --- proxy/logging/LogConfig.cc | 4 +-- proxy/logging/LogFile.cc | 4 ++- src/traffic_server/traffic_server.cc | 8 ++++- .../gold_tests/logging/log_retention.test.py | 34 ++++++++++++------- 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/proxy/logging/LogConfig.cc b/proxy/logging/LogConfig.cc index 7676a6e3ab7..24fabecd1c9 100644 --- a/proxy/logging/LogConfig.cc +++ b/proxy/logging/LogConfig.cc @@ -70,7 +70,7 @@ LogConfig::setup_default_values() { const unsigned int bufSize = 512; char name[bufSize]; - if (!gethostname(name, bufSize)) { + if (gethostname(name, bufSize) == -1) { ink_strlcpy(name, "unknown_host_name", sizeof(name)); } hostname = ats_strdup(name); @@ -160,7 +160,7 @@ LogConfig::read_configuration_variables() ats_free(ptr); ptr = REC_ConfigReadString("proxy.config.log.hostname"); - if (ptr != nullptr) { + if (ptr != nullptr && std::string_view(ptr) != "localhost") { ats_free(hostname); hostname = ptr; } diff --git a/proxy/logging/LogFile.cc b/proxy/logging/LogFile.cc index 7f5ca6c5f40..15b62ee38da 100644 --- a/proxy/logging/LogFile.cc +++ b/proxy/logging/LogFile.cc @@ -73,7 +73,9 @@ LogFile::LogFile(const char *name, const char *header, LogFileFormat format, uin { if (m_file_format != LOG_FILE_PIPE) { m_log = new BaseLogFile(name, m_signature); - m_log->set_hostname(Machine::instance()->hostname); + // Use Log::config->hostname rather than Machine::instance()->hostname + // because the former is reloadable. + m_log->set_hostname(Log::config->hostname); } else { m_log = nullptr; } diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index 887eecbb317..6f92d5936a7 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -1943,7 +1943,13 @@ main(int /* argc ATS_UNUSED */, const char **argv) } else if (HttpConfig::m_master.inbound_ip6.isValid()) { machine_addr.assign(HttpConfig::m_master.inbound_ip6); } - Machine::init(nullptr, &machine_addr.sa); + char *hostname = REC_ConfigReadString("proxy.config.log.hostname"); + if (hostname != nullptr && std::string_view(hostname) == "localhost") { + // The default value was used. Let Machine::init derive the hostname. + hostname = nullptr; + } + Machine::init(hostname, &machine_addr.sa); + ats_free(hostname); RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.uuid", (char *)Machine::instance()->uuid.getString(), RECP_NON_PERSISTENT); diff --git a/tests/gold_tests/logging/log_retention.test.py b/tests/gold_tests/logging/log_retention.test.py index 981af771e92..605bfd84a26 100644 --- a/tests/gold_tests/logging/log_retention.test.py +++ b/tests/gold_tests/logging/log_retention.test.py @@ -18,6 +18,7 @@ # limitations under the License. import os +import socket Test.Summary = ''' Test the enforcement of proxy.config.log.max_space_mb_for_logs. @@ -46,7 +47,7 @@ class TestLogRetention: } __server = None - __ts_counter = 1 + __ts_counter = 0 __server_is_started = False def __init__(self, records_config, run_description, command="traffic_manager"): @@ -144,14 +145,17 @@ def get_command_to_rotate_thrice(self): # -# Run 1: Verify that log deletion happens when no min_count is specified. +# Test 0: Verify that log deletion happens when no min_count is specified. # +specified_hostname = 'my_hostname' twelve_meg_log_space = { # The following configures a 12 MB log cap with a required 2 MB head room. # Thus the rotated log of just over 10 MB should be deleted because it # will not leave enough head room. 'proxy.config.log.max_space_mb_headroom': 2, 'proxy.config.log.max_space_mb_for_logs': 12, + # Verify that setting a hostname changes the hostname used in rolled logs. + 'proxy.config.log.hostname': specified_hostname, } test = TestLogRetention(twelve_meg_log_space, "Verify log rotation and deletion of the configured log file with no min_count.") @@ -187,7 +191,7 @@ def get_command_to_rotate_thrice(self): "Verify manager.log auto-delete configuration") # Verify test_deletion was rotated and deleted. test.ts.Streams.stderr += Testers.ContainsExpression( - "The rolled logfile.*test_deletion.log_.*was auto-deleted.*bytes were reclaimed", + f"The rolled logfile.*test_deletion.log_{specified_hostname}.*was auto-deleted.*bytes were reclaimed", "Verify that space was reclaimed") test.tr.Processes.Default.Command = test.get_command_to_rotate_once() @@ -198,7 +202,7 @@ def get_command_to_rotate_thrice(self): # -# Test 2: Verify log deletion happens with a min_count of 1. +# Test 1: Verify log deletion happens with a min_count of 1. # test = TestLogRetention(twelve_meg_log_space, "Verify log rotation and deletion of the configured log file with a min_count of 1.") @@ -236,7 +240,7 @@ def get_command_to_rotate_thrice(self): "Verify manager.log auto-delete configuration") # Verify test_deletion was rotated and deleted. test.ts.Streams.stderr += Testers.ContainsExpression( - "The rolled logfile.*test_deletion.log_.*was auto-deleted.*bytes were reclaimed", + f"The rolled logfile.*test_deletion.log_{specified_hostname}.*was auto-deleted.*bytes were reclaimed", "Verify that space was reclaimed") test.tr.Processes.Default.Command = test.get_command_to_rotate_once() @@ -246,7 +250,7 @@ def get_command_to_rotate_thrice(self): # -# Test 3: Verify log deletion happens for a plugin's logs. +# Test 2: Verify log deletion happens for a plugin's logs. # test = TestLogRetention(twelve_meg_log_space, "Verify log rotation and deletion of plugin logs.") @@ -279,7 +283,7 @@ def get_command_to_rotate_thrice(self): test.tr.StillRunningAfter = test.server # -# Test 4: Verify log deletion priority behavior. +# Test 3: Verify log deletion priority behavior. # twenty_two_meg_log_space = { # The following configures a 22 MB log cap with a required 2 MB head room. @@ -332,8 +336,12 @@ def get_command_to_rotate_thrice(self): test.ts.Streams.stderr += Testers.ExcludesExpression( "The rolled logfile.*test_low_priority_deletion.log_.*was auto-deleted.*bytes were reclaimed", "Verify that space was reclaimed from test_high_priority_deletion") + +# Verify that ATS derives the hostname correctly if the user does not specify a +# hostname via 'proxy.config.log.hostname'. +hostname = socket.gethostname() test.ts.Streams.stderr += Testers.ContainsExpression( - "The rolled logfile.*test_high_priority_deletion.log_.*was auto-deleted.*bytes were reclaimed", + f"The rolled logfile.*test_high_priority_deletion.log_{hostname}.*was auto-deleted.*bytes were reclaimed", "Verify that space was reclaimed from test_high_priority_deletion") test.tr.Processes.Default.Command = test.get_command_to_rotate_once() @@ -342,7 +350,7 @@ def get_command_to_rotate_thrice(self): test.tr.StillRunningAfter = test.server # -# Test 5: Verify min_count configuration overrides. +# Test 4: Verify min_count configuration overrides. # various_min_count_overrides = { 'proxy.config.log.max_space_mb_for_logs': 22, @@ -381,11 +389,13 @@ def get_command_to_rotate_thrice(self): # -# Test 6: Verify log deletion does not happen when it is disabled. +# Test 5: Verify log deletion does not happen when it is disabled. # auto_delete_disabled = twelve_meg_log_space.copy() auto_delete_disabled.update({ 'proxy.config.log.auto_delete_rolled_files': 0, + # Verify that setting a hostname changes the hostname used in rolled logs. + 'proxy.config.log.hostname': 'my_hostname', }) test = TestLogRetention(auto_delete_disabled, "Verify log deletion does not happen when auto-delet is disabled.") @@ -432,7 +442,7 @@ def get_command_to_rotate_thrice(self): test.tr.StillRunningAfter = test.server # -# Test 7: Verify that max_roll_count is respected. +# Test 6: Verify that max_roll_count is respected. # max_roll_count_of_2 = { 'proxy.config.diags.debug.tags': 'log-file', @@ -472,7 +482,7 @@ def get_command_to_rotate_thrice(self): test.tr.StillRunningAfter = test.server # -# Test 8: Verify log deletion happens after a config reload. +# Test 7: Verify log deletion happens after a config reload. # test = TestLogRetention(twelve_meg_log_space, "Verify log rotation and deletion after a config reload.") From 388a3a79ea285e25a64d7f450319391bdbe658e3 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Fri, 18 Jun 2021 21:57:54 +0000 Subject: [PATCH 2/2] Use Machine::instance()->hostname instead of gethostname This adds some assurance that my code change isn't altering any of the log naming behavior. Log names will still, as before, be Machine::instance()->hostname. --- proxy/logging/LogConfig.cc | 9 ++------- proxy/shared/UglyLogStubs.cc | 22 ++++++++++++++++++++-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/proxy/logging/LogConfig.cc b/proxy/logging/LogConfig.cc index 24fabecd1c9..37d86758790 100644 --- a/proxy/logging/LogConfig.cc +++ b/proxy/logging/LogConfig.cc @@ -23,6 +23,7 @@ #include "tscore/ink_platform.h" #include "tscore/I_Layout.h" +#include "I_Machine.h" #ifdef HAVE_SYS_PARAM_H #include @@ -68,13 +69,7 @@ void LogConfig::setup_default_values() { - const unsigned int bufSize = 512; - char name[bufSize]; - if (gethostname(name, bufSize) == -1) { - ink_strlcpy(name, "unknown_host_name", sizeof(name)); - } - hostname = ats_strdup(name); - + hostname = ats_strdup(Machine::instance()->hostname); log_buffer_size = static_cast(10 * LOG_KILOBYTE); max_secs_per_buffer = 5; max_space_mb_for_logs = 100; diff --git a/proxy/shared/UglyLogStubs.cc b/proxy/shared/UglyLogStubs.cc index f30b9545853..87a95b40cbf 100644 --- a/proxy/shared/UglyLogStubs.cc +++ b/proxy/shared/UglyLogStubs.cc @@ -66,13 +66,31 @@ ConfigUpdateCbTable::invoke(const char * /* name ATS_UNUSED */) } struct Machine { + Machine(); + ~Machine(); static Machine *instance(); + char *hostname = nullptr; + +private: + static Machine _instance; }; + +Machine Machine::_instance; + +Machine::Machine() +{ + hostname = ats_strdup("test.host.com"); +} + +Machine::~Machine() +{ + ats_free(hostname); +} + Machine * Machine::instance() { - ink_release_assert(false); - return nullptr; + return &_instance; } NetAccept *