From 9f28180ae4bebfb529a8a7f3bf6758cb44a3c57a Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Mon, 3 Oct 2016 14:05:57 -0500 Subject: [PATCH 01/11] TS-4399 TS-4400 Management API messes up proxy options TS-4399: Management API breaks diagnostic log rotation TS-4400: TSProxyStateSet persist cache clearing across restart The two issues are related in that they both deal with the management API not correctly handling proxy flags. For TS-4399, it was because the management API was not aware of traffic_manager setting extra proxy options. This was fixed by providing CoreAPI a callback to get extra proxy options from traffic_manager. For TS-4400, it was because the management API was not properly clearing optional flags between proxy reboots. This was fixed by resetting the proxy options before each reboot. --- cmd/traffic_manager/traffic_manager.cc | 22 ++++++++++++++++++++++ mgmt/api/CoreAPI.cc | 26 ++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index 143915a1482..081449ef291 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -27,6 +27,7 @@ #include "ts/ink_sock.h" #include "ts/ink_args.h" #include "ts/ink_syslog.h" +#include #include "WebMgmtUtils.h" #include "WebOverview.h" @@ -65,6 +66,9 @@ #define FD_THROTTLE_HEADROOM (128 + 64) // TODO: consolidate with THROTTLE_FD_HEADROOM #define DIAGS_LOG_FILENAME "manager.log" +// Used to pass traffic_server options to the RPC API +extern std::function&)> proxy_options_callback; // declared in CoreAPI.cc + // These globals are still referenced directly by management API. LocalManager *lmgmt = NULL; FileManager *configFiles; @@ -445,6 +449,8 @@ main(int argc, const char **argv) int binding_version = 0; BindingInstance *binding = NULL; + std::vector callback_proxy_options; + ArgumentDescription argument_descriptions[] = { {"proxyOff", '-', "Disable proxy", "F", &proxy_off, NULL, NULL}, {"aconfPort", '-', "Autoconf port", "I", &aconf_port_arg, "MGMT_ACONF_PORT", NULL}, @@ -619,6 +625,9 @@ main(int argc, const char **argv) // we must pass in bind_stdout and bind_stderr values to TS // we do it so TS is able to create BaseLogFiles for each value if (*bind_stdout != 0) { + callback_proxy_options.push_back(ats_strdup(TM_OPT_BIND_STDOUT)); + callback_proxy_options.push_back(ats_strdup(bind_stdout)); + size_t l = strlen(lmgmt->proxy_options); size_t n = 3 /* " --" */ + sizeof(TM_OPT_BIND_STDOUT) /* nul accounted for here */ @@ -629,6 +638,9 @@ main(int argc, const char **argv) } if (*bind_stderr != 0) { + callback_proxy_options.push_back(ats_strdup(TM_OPT_BIND_STDERR)); + callback_proxy_options.push_back(ats_strdup(bind_stderr)); + size_t l = strlen(lmgmt->proxy_options); size_t n = 3 /* space dash dash */ + sizeof(TM_OPT_BIND_STDERR) /* nul accounted for here */ @@ -638,6 +650,16 @@ main(int argc, const char **argv) snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDERR, bind_stderr); } + // If there exist proxy options we want CoreAPI to have, we set the callback + // to a lambda that will pass the options + if (callback_proxy_options.size() > 0) { + proxy_options_callback = [&callback_proxy_options](std::vector &opts) { + for (auto it = callback_proxy_options.begin(); it < callback_proxy_options.end(); ++it) { + opts.push_back(ats_strdup(*it)); + } + }; + } + if (proxy_port) { HttpProxyPort::loadValue(lmgmt->m_proxy_ports, proxy_port); } diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index 9e4f689c286..4865053ce87 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -41,6 +41,7 @@ #include "ts/Diags.h" #include "ts/ink_hash_table.h" #include "ExpandingArray.h" +#include //#include "I_AccCrypto.h" #include "CoreAPI.h" @@ -53,6 +54,9 @@ // global variable CallbackTable *local_event_callbacks; +// Used to get any proxy options that traffic_manager might want to tack on +std::function&)> proxy_options_callback; + extern FileManager *configFiles; // global in traffic_manager /*------------------------------------------------------------------------- @@ -184,12 +188,26 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear) ink_strlcat(tsArgs, " -k", sizeof(tsArgs)); } - if (strlen(tsArgs) > 0) { /* Passed command line args for proxy */ - ats_free(lmgmt->proxy_options); - lmgmt->proxy_options = ats_strdup(tsArgs); - mgmt_log("[ProxyStateSet] Traffic Server Args: '%s'\n", lmgmt->proxy_options); + // If we've been provided an options callback, then we call the callback and + // put all the extra options we receive into tsArgs + // + // proxy_options_callback is given to us by traffic_manager + if (proxy_options_callback) { + std::vector options; + proxy_options_callback(options); + + for (auto it = options.begin(); it < options.end(); ++it) { + ink_strlcat(tsArgs, " ", sizeof(tsArgs)); + ink_strlcat(tsArgs, static_cast(*it), sizeof(tsArgs)); + } } + // We always want to delete the previous run's proxy_options + // because we to repopulate with the most up to date options each TS restart + ats_free(lmgmt->proxy_options); + lmgmt->proxy_options = ats_strdup(tsArgs); + mgmt_log("[ProxyStateSet] Traffic Server Args: '%s'\n", lmgmt->proxy_options); + lmgmt->run_proxy = true; lmgmt->listenForProxy(); From 3365b2048e7bf0660faccf573ffd198857d03d25 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Mon, 3 Oct 2016 15:14:24 -0500 Subject: [PATCH 02/11] Fix memory leaks --- cmd/traffic_manager/traffic_manager.cc | 5 +++++ mgmt/api/CoreAPI.cc | 1 + 2 files changed, 6 insertions(+) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index 081449ef291..f0a4701b300 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -866,6 +866,11 @@ main(int argc, const char **argv) } } + // Delete the proxy options we saved for CoreAPI. + // This probably isn't needed b/c the process dies here... + for (auto it = callback_proxy_options.begin(); it < callback_proxy_options.end(); ++it) + free(*it); + if (binding) { metrics_binding_destroy(*binding); delete binding; diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index 4865053ce87..96a4bce3717 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -199,6 +199,7 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear) for (auto it = options.begin(); it < options.end(); ++it) { ink_strlcat(tsArgs, " ", sizeof(tsArgs)); ink_strlcat(tsArgs, static_cast(*it), sizeof(tsArgs)); + free(*it); } } From 82037192bf899a7c94b1b61b2181e17e8088bef9 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Fri, 14 Oct 2016 11:40:39 -0500 Subject: [PATCH 03/11] Centralize traffic_server entry point This patch centralizes where traffic_server starts to inside CoreAPI's ProxyStateSet. This is good because we reduce the number of places we deal with traffic_server options. Everything is simply handled by the proxy_options_callback. Note: Right now, there is about a ~30 delay between ATS starting up and traffic_ctl commands working. Not sure why. --- cmd/traffic_manager/traffic_manager.cc | 42 ++++++++++---------------- mgmt/api/CoreAPI.cc | 1 + 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index f0a4701b300..03cad20938c 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -616,38 +616,27 @@ main(int argc, const char **argv) RecLocalStart(configFiles); /* Update cmd line overrides/environmental overrides/etc */ - if (tsArgs) { /* Passed command line args for proxy */ - ats_free(lmgmt->proxy_options); - lmgmt->proxy_options = tsArgs; - mgmt_log("[main] Traffic Server Args: '%s'\n", lmgmt->proxy_options); - } + if (tsArgs) /* Passed command line args for proxy */ + callback_proxy_options.push_back(ats_strdup(tsArgs)); - // we must pass in bind_stdout and bind_stderr values to TS - // we do it so TS is able to create BaseLogFiles for each value + // we must give bind_stdout and bind_stderr values to callback_proxy_options + // so LocalManager can start up TS with the correct options + // + // TS needs them to be able to create BaseLogFiles for each value if (*bind_stdout != 0) { - callback_proxy_options.push_back(ats_strdup(TM_OPT_BIND_STDOUT)); + char *bind_stdout_opt = new char[strlen("--") + strlen(TM_OPT_BIND_STDOUT)]; + strcpy(bind_stdout_opt, "--"); + strcat(bind_stdout_opt, TM_OPT_BIND_STDOUT); + callback_proxy_options.push_back(bind_stdout_opt); callback_proxy_options.push_back(ats_strdup(bind_stdout)); - - size_t l = strlen(lmgmt->proxy_options); - size_t n = 3 /* " --" */ - + sizeof(TM_OPT_BIND_STDOUT) /* nul accounted for here */ - + 1 /* space */ - + strlen(bind_stdout); - lmgmt->proxy_options = static_cast(ats_realloc(lmgmt->proxy_options, n + l)); - snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDOUT, bind_stdout); } if (*bind_stderr != 0) { - callback_proxy_options.push_back(ats_strdup(TM_OPT_BIND_STDERR)); + char *bind_stderr_opt = new char[strlen("--") + strlen(TM_OPT_BIND_STDERR)]; + strcpy(bind_stderr_opt, "--"); + strcat(bind_stderr_opt, TM_OPT_BIND_STDERR); + callback_proxy_options.push_back(bind_stderr_opt); callback_proxy_options.push_back(ats_strdup(bind_stderr)); - - size_t l = strlen(lmgmt->proxy_options); - size_t n = 3 /* space dash dash */ - + sizeof(TM_OPT_BIND_STDERR) /* nul accounted for here */ - + 1 /* space */ - + strlen(bind_stderr); - lmgmt->proxy_options = static_cast(ats_realloc(lmgmt->proxy_options, n + l)); - snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDERR, bind_stderr); } // If there exist proxy options we want CoreAPI to have, we set the callback @@ -835,10 +824,11 @@ main(int argc, const char **argv) } else { sleep_time = 1; } - if (lmgmt->startProxy()) { + if (ProxyStateSet(TS_PROXY_ON, TS_CACHE_CLEAR_NONE) == TS_ERR_OKAY) { just_started = 0; sleep_time = 0; } else { + mgmt_log("in ProxyStateSet else branch"); just_started++; } } else { /* Give the proxy a chance to fire up */ diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index 96a4bce3717..8119452ed90 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -210,6 +210,7 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear) mgmt_log("[ProxyStateSet] Traffic Server Args: '%s'\n", lmgmt->proxy_options); lmgmt->run_proxy = true; + lmgmt->startProxy(); lmgmt->listenForProxy(); do { From af59ccb2ad2719f928b068341380747f6dfa9f49 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Wed, 19 Oct 2016 16:12:05 -0500 Subject: [PATCH 04/11] Remove callback between traffic_manager and LocalManager Instead, use proxy_options as persistent proxy options storage. Then have lmgmt->startProxy() take in an argument for one-time flags. This was done to reduce complicated dependencies. --- cmd/traffic_manager/traffic_manager.cc | 50 ++++++++++++-------------- mgmt/LocalManager.cc | 14 ++++++-- mgmt/LocalManager.h | 4 +-- mgmt/api/CoreAPI.cc | 23 ++---------- 4 files changed, 38 insertions(+), 53 deletions(-) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index 03cad20938c..b5bbbc1b34d 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -66,9 +66,6 @@ #define FD_THROTTLE_HEADROOM (128 + 64) // TODO: consolidate with THROTTLE_FD_HEADROOM #define DIAGS_LOG_FILENAME "manager.log" -// Used to pass traffic_server options to the RPC API -extern std::function&)> proxy_options_callback; // declared in CoreAPI.cc - // These globals are still referenced directly by management API. LocalManager *lmgmt = NULL; FileManager *configFiles; @@ -616,37 +613,36 @@ main(int argc, const char **argv) RecLocalStart(configFiles); /* Update cmd line overrides/environmental overrides/etc */ - if (tsArgs) /* Passed command line args for proxy */ - callback_proxy_options.push_back(ats_strdup(tsArgs)); + if (tsArgs) { /* Passed command line args for proxy */ + char* new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(tsArgs) + 1]; + strcpy(new_proxy_opts, lmgmt->proxy_options); + strcat(new_proxy_opts, tsArgs); + ats_free(lmgmt->proxy_options); + lmgmt->proxy_options = new_proxy_opts; + } - // we must give bind_stdout and bind_stderr values to callback_proxy_options - // so LocalManager can start up TS with the correct options + // TS needs to be started up with the same outputlog bindings each time, + // so we append the outputlog location to the persistent proxy options // // TS needs them to be able to create BaseLogFiles for each value if (*bind_stdout != 0) { - char *bind_stdout_opt = new char[strlen("--") + strlen(TM_OPT_BIND_STDOUT)]; - strcpy(bind_stdout_opt, "--"); - strcat(bind_stdout_opt, TM_OPT_BIND_STDOUT); - callback_proxy_options.push_back(bind_stdout_opt); - callback_proxy_options.push_back(ats_strdup(bind_stdout)); + const char *bind_stdout_opt = " --" TM_OPT_BIND_STDOUT " "; + char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stdout_opt) + strlen(bind_stdout) + 1]; + strcpy(new_proxy_opts, lmgmt->proxy_options); + strcat(new_proxy_opts, bind_stdout_opt); + strcat(new_proxy_opts, bind_stdout); + delete lmgmt->proxy_options; + lmgmt->proxy_options = new_proxy_opts; } if (*bind_stderr != 0) { - char *bind_stderr_opt = new char[strlen("--") + strlen(TM_OPT_BIND_STDERR)]; - strcpy(bind_stderr_opt, "--"); - strcat(bind_stderr_opt, TM_OPT_BIND_STDERR); - callback_proxy_options.push_back(bind_stderr_opt); - callback_proxy_options.push_back(ats_strdup(bind_stderr)); - } - - // If there exist proxy options we want CoreAPI to have, we set the callback - // to a lambda that will pass the options - if (callback_proxy_options.size() > 0) { - proxy_options_callback = [&callback_proxy_options](std::vector &opts) { - for (auto it = callback_proxy_options.begin(); it < callback_proxy_options.end(); ++it) { - opts.push_back(ats_strdup(*it)); - } - }; + const char *bind_stderr_opt = " --" TM_OPT_BIND_STDERR " "; + char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stderr_opt) + strlen(bind_stderr) + 1]; + strcpy(new_proxy_opts, lmgmt->proxy_options); + strcat(new_proxy_opts, bind_stderr_opt); + strcat(new_proxy_opts, bind_stderr); + delete lmgmt->proxy_options; + lmgmt->proxy_options = new_proxy_opts; } if (proxy_port) { diff --git a/mgmt/LocalManager.cc b/mgmt/LocalManager.cc index 67cad23d218..d3370b2a037 100644 --- a/mgmt/LocalManager.cc +++ b/mgmt/LocalManager.cc @@ -239,8 +239,9 @@ LocalManager::LocalManager(bool proxy_on) : BaseManager(), run_proxy(proxy_on), process_server_timeout_msecs = REC_readInteger("proxy.config.lm.pserver_timeout_msecs", &found); proxy_name = REC_readString("proxy.config.proxy_name", &found); proxy_binary = REC_readString("proxy.config.proxy_binary", &found); - proxy_options = REC_readString("proxy.config.proxy_binary_opts", &found); env_prep = REC_readString("proxy.config.env_prep", &found); + proxy_options = new char[1]; + strcpy(proxy_options, ""); // so later we can always `delete proxy_options` without worrying // Calculate proxy_binary from the absolute bin_path absolute_proxy_binary = Layout::relative_to(bindir, proxy_binary); @@ -831,9 +832,13 @@ LocalManager::processEventQueue() /* * startProxy() * Function fires up a proxy process. + * + * Args: + * onetime_options: one time options that traffic_server should be started with (ie + * these options do not persist across reboots) */ bool -LocalManager::startProxy() +LocalManager::startProxy(char *onetime_options) { if (proxy_launch_outstanding) { return false; @@ -902,8 +907,11 @@ LocalManager::startProxy() Vec real_proxy_options; real_proxy_options.append(proxy_options, strlen(proxy_options)); + real_proxy_options.append(" ", strlen(" ")); + real_proxy_options.append(onetime_options, strlen(onetime_options)); - if (!strstr(proxy_options, MGMT_OPT)) { // Make sure we're starting the proxy in mgmt mode + // Make sure we're starting the proxy in mgmt mode + if (!strstr(proxy_options, MGMT_OPT) && !strstr(onetime_options, MGMT_OPT)) { real_proxy_options.append(" ", strlen(" ")); real_proxy_options.append(MGMT_OPT, sizeof(MGMT_OPT) - 1); } diff --git a/mgmt/LocalManager.h b/mgmt/LocalManager.h index c9e99ded321..e59aadd7469 100644 --- a/mgmt/LocalManager.h +++ b/mgmt/LocalManager.h @@ -74,7 +74,7 @@ class LocalManager : public BaseManager void signalAlarm(int alarm_id, const char *desc = NULL, const char *ip = NULL); void processEventQueue(); - bool startProxy(); + bool startProxy(char *onetime_options); void listenForProxy(); void bindProxyPort(HttpProxyPort &); void closeProxyPorts(); @@ -108,7 +108,7 @@ class LocalManager : public BaseManager char *absolute_proxy_binary; char *proxy_name; char *proxy_binary; - char *proxy_options; + char *proxy_options; // These options should persist across proxy reboots char *env_prep; int process_server_sockfd; diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index 8119452ed90..014afedf73b 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -188,30 +188,11 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear) ink_strlcat(tsArgs, " -k", sizeof(tsArgs)); } - // If we've been provided an options callback, then we call the callback and - // put all the extra options we receive into tsArgs - // - // proxy_options_callback is given to us by traffic_manager - if (proxy_options_callback) { - std::vector options; - proxy_options_callback(options); - - for (auto it = options.begin(); it < options.end(); ++it) { - ink_strlcat(tsArgs, " ", sizeof(tsArgs)); - ink_strlcat(tsArgs, static_cast(*it), sizeof(tsArgs)); - free(*it); - } - } - - // We always want to delete the previous run's proxy_options - // because we to repopulate with the most up to date options each TS restart - ats_free(lmgmt->proxy_options); - lmgmt->proxy_options = ats_strdup(tsArgs); - mgmt_log("[ProxyStateSet] Traffic Server Args: '%s'\n", lmgmt->proxy_options); + mgmt_log("[ProxyStateSet] Traffic Server Args: '%s %s'\n", lmgmt->proxy_options ? lmgmt->proxy_options : "", tsArgs); lmgmt->run_proxy = true; - lmgmt->startProxy(); lmgmt->listenForProxy(); + lmgmt->startProxy(tsArgs); do { mgmt_sleep_sec(1); From 79b2b706426faa4848ceadc48035de6e696941c3 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Fri, 28 Oct 2016 15:10:35 -0500 Subject: [PATCH 05/11] Code cleanup --- cmd/traffic_manager/traffic_manager.cc | 40 +++++++------------------- mgmt/LocalManager.cc | 11 +++---- mgmt/LocalManager.h | 2 +- mgmt/api/CoreAPI.cc | 4 --- 4 files changed, 17 insertions(+), 40 deletions(-) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index b5bbbc1b34d..4d26adf227b 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -27,7 +27,6 @@ #include "ts/ink_sock.h" #include "ts/ink_args.h" #include "ts/ink_syslog.h" -#include #include "WebMgmtUtils.h" #include "WebOverview.h" @@ -38,6 +37,7 @@ #include "FileManager.h" #include "ts/I_Layout.h" #include "ts/I_Version.h" +#include "ts/TextBuffer.h" #include "DiagsConfig.h" #include "HTTP.h" #include "CoreAPI.h" @@ -446,8 +446,6 @@ main(int argc, const char **argv) int binding_version = 0; BindingInstance *binding = NULL; - std::vector callback_proxy_options; - ArgumentDescription argument_descriptions[] = { {"proxyOff", '-', "Disable proxy", "F", &proxy_off, NULL, NULL}, {"aconfPort", '-', "Autoconf port", "I", &aconf_port_arg, "MGMT_ACONF_PORT", NULL}, @@ -614,36 +612,23 @@ main(int argc, const char **argv) /* Update cmd line overrides/environmental overrides/etc */ if (tsArgs) { /* Passed command line args for proxy */ - char* new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(tsArgs) + 1]; - strcpy(new_proxy_opts, lmgmt->proxy_options); - strcat(new_proxy_opts, tsArgs); - ats_free(lmgmt->proxy_options); - lmgmt->proxy_options = new_proxy_opts; + lmgmt->proxy_options = ats_strdup(tsArgs); } // TS needs to be started up with the same outputlog bindings each time, // so we append the outputlog location to the persistent proxy options // // TS needs them to be able to create BaseLogFiles for each value - if (*bind_stdout != 0) { - const char *bind_stdout_opt = " --" TM_OPT_BIND_STDOUT " "; - char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stdout_opt) + strlen(bind_stdout) + 1]; - strcpy(new_proxy_opts, lmgmt->proxy_options); - strcat(new_proxy_opts, bind_stdout_opt); - strcat(new_proxy_opts, bind_stdout); - delete lmgmt->proxy_options; - lmgmt->proxy_options = new_proxy_opts; + textBuffer args(1024); + if (*bind_stdout) { + const char *space = args.empty() ? "" : " "; + args.format("%s%s %s", space, "--" TM_OPT_BIND_STDOUT, bind_stdout); } - - if (*bind_stderr != 0) { - const char *bind_stderr_opt = " --" TM_OPT_BIND_STDERR " "; - char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stderr_opt) + strlen(bind_stderr) + 1]; - strcpy(new_proxy_opts, lmgmt->proxy_options); - strcat(new_proxy_opts, bind_stderr_opt); - strcat(new_proxy_opts, bind_stderr); - delete lmgmt->proxy_options; - lmgmt->proxy_options = new_proxy_opts; + if (*bind_stderr) { + const char *space = args.empty() ? "" : " "; + args.format("%s%s %s", space, "--" TM_OPT_BIND_STDERR, bind_stderr); } + lmgmt->proxy_options = args.release(); if (proxy_port) { HttpProxyPort::loadValue(lmgmt->m_proxy_ports, proxy_port); @@ -852,11 +837,6 @@ main(int argc, const char **argv) } } - // Delete the proxy options we saved for CoreAPI. - // This probably isn't needed b/c the process dies here... - for (auto it = callback_proxy_options.begin(); it < callback_proxy_options.end(); ++it) - free(*it); - if (binding) { metrics_binding_destroy(*binding); delete binding; diff --git a/mgmt/LocalManager.cc b/mgmt/LocalManager.cc index d3370b2a037..9e080a3cebc 100644 --- a/mgmt/LocalManager.cc +++ b/mgmt/LocalManager.cc @@ -240,8 +240,7 @@ LocalManager::LocalManager(bool proxy_on) : BaseManager(), run_proxy(proxy_on), proxy_name = REC_readString("proxy.config.proxy_name", &found); proxy_binary = REC_readString("proxy.config.proxy_binary", &found); env_prep = REC_readString("proxy.config.env_prep", &found); - proxy_options = new char[1]; - strcpy(proxy_options, ""); // so later we can always `delete proxy_options` without worrying + proxy_options = NULL; // Calculate proxy_binary from the absolute bin_path absolute_proxy_binary = Layout::relative_to(bindir, proxy_binary); @@ -838,7 +837,7 @@ LocalManager::processEventQueue() * these options do not persist across reboots) */ bool -LocalManager::startProxy(char *onetime_options) +LocalManager::startProxy(const char *onetime_options) { if (proxy_launch_outstanding) { return false; @@ -907,8 +906,10 @@ LocalManager::startProxy(char *onetime_options) Vec real_proxy_options; real_proxy_options.append(proxy_options, strlen(proxy_options)); - real_proxy_options.append(" ", strlen(" ")); - real_proxy_options.append(onetime_options, strlen(onetime_options)); + if (onetime_options && *onetime_options) { + real_proxy_options.append(" ", strlen(" ")); + real_proxy_options.append(onetime_options, strlen(onetime_options)); + } // Make sure we're starting the proxy in mgmt mode if (!strstr(proxy_options, MGMT_OPT) && !strstr(onetime_options, MGMT_OPT)) { diff --git a/mgmt/LocalManager.h b/mgmt/LocalManager.h index e59aadd7469..dcc67617d8f 100644 --- a/mgmt/LocalManager.h +++ b/mgmt/LocalManager.h @@ -74,7 +74,7 @@ class LocalManager : public BaseManager void signalAlarm(int alarm_id, const char *desc = NULL, const char *ip = NULL); void processEventQueue(); - bool startProxy(char *onetime_options); + bool startProxy(const char *onetime_options); void listenForProxy(); void bindProxyPort(HttpProxyPort &); void closeProxyPorts(); diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index 014afedf73b..ed785fa3c63 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -41,7 +41,6 @@ #include "ts/Diags.h" #include "ts/ink_hash_table.h" #include "ExpandingArray.h" -#include //#include "I_AccCrypto.h" #include "CoreAPI.h" @@ -54,9 +53,6 @@ // global variable CallbackTable *local_event_callbacks; -// Used to get any proxy options that traffic_manager might want to tack on -std::function&)> proxy_options_callback; - extern FileManager *configFiles; // global in traffic_manager /*------------------------------------------------------------------------- From 586b8ff57c9fd2ac69edf0508fdd4fcf782e9344 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Mon, 31 Oct 2016 11:33:11 -0500 Subject: [PATCH 06/11] Fix CoreAPI sleeping deadlock This change was made because the ProxyStateSet() call was sleeping in the same thread as traffic_manager was running, so the condition in the do/while loop could never be updated by traffic_manager. The solution is to differentiate API calls and local calls to CoreAPI by using the coreapi_sleep flag. --- cmd/traffic_manager/traffic_manager.cc | 2 ++ mgmt/LocalManager.cc | 1 + mgmt/LocalManager.h | 1 + mgmt/api/CoreAPI.cc | 11 ++++++----- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index 4d26adf227b..0c719ec4442 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -805,6 +805,7 @@ main(int argc, const char **argv) } else { sleep_time = 1; } + lmgmt->coreapi_sleep = false; if (ProxyStateSet(TS_PROXY_ON, TS_CACHE_CLEAR_NONE) == TS_ERR_OKAY) { just_started = 0; sleep_time = 0; @@ -812,6 +813,7 @@ main(int argc, const char **argv) mgmt_log("in ProxyStateSet else branch"); just_started++; } + lmgmt->coreapi_sleep = true; } else { /* Give the proxy a chance to fire up */ just_started++; } diff --git a/mgmt/LocalManager.cc b/mgmt/LocalManager.cc index 9e080a3cebc..50eae96cd9f 100644 --- a/mgmt/LocalManager.cc +++ b/mgmt/LocalManager.cc @@ -189,6 +189,7 @@ LocalManager::LocalManager(bool proxy_on) : BaseManager(), run_proxy(proxy_on), proxy_launch_outstanding = false; mgmt_shutdown_outstanding = MGMT_PENDING_NONE; proxy_running = 0; + coreapi_sleep = true; RecRegisterStatInt(RECT_NODE, "proxy.node.proxy_running", 0, RECP_NON_PERSISTENT); diff --git a/mgmt/LocalManager.h b/mgmt/LocalManager.h index dcc67617d8f..1f859fa3f3c 100644 --- a/mgmt/LocalManager.h +++ b/mgmt/LocalManager.h @@ -90,6 +90,7 @@ class LocalManager : public BaseManager bool processRunning(); bool clusterOk(); + volatile bool coreapi_sleep; volatile bool run_proxy; volatile time_t manager_started_at; volatile time_t proxy_started_at; diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index ed785fa3c63..88da63dce62 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -190,12 +190,13 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear) lmgmt->listenForProxy(); lmgmt->startProxy(tsArgs); - do { - mgmt_sleep_sec(1); - } while (i++ < 20 && (lmgmt->proxy_running == 0)); + if (lmgmt->coreapi_sleep) { + do { + mgmt_sleep_sec(1); + } while (i++ < 20 && (lmgmt->proxy_running == 0)); - if (!lmgmt->processRunning()) { - goto Lerror; + if (!lmgmt->processRunning()) + goto Lerror; } break; From 0fe5db316153bafab15e97899b1474f9eed00f02 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Mon, 31 Oct 2016 11:35:42 -0500 Subject: [PATCH 07/11] clang-format --- mgmt/LocalManager.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mgmt/LocalManager.h b/mgmt/LocalManager.h index 1f859fa3f3c..1c96b22471c 100644 --- a/mgmt/LocalManager.h +++ b/mgmt/LocalManager.h @@ -109,7 +109,7 @@ class LocalManager : public BaseManager char *absolute_proxy_binary; char *proxy_name; char *proxy_binary; - char *proxy_options; // These options should persist across proxy reboots + char *proxy_options; // These options should persist across proxy reboots char *env_prep; int process_server_sockfd; From 10c10af585d246dfba8a579ae6542793f2bb2b05 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Mon, 31 Oct 2016 11:37:06 -0500 Subject: [PATCH 08/11] Code cleanup --- cmd/traffic_manager/traffic_manager.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index 0c719ec4442..b8f4d12f65e 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -810,7 +810,6 @@ main(int argc, const char **argv) just_started = 0; sleep_time = 0; } else { - mgmt_log("in ProxyStateSet else branch"); just_started++; } lmgmt->coreapi_sleep = true; From 5a989cb68d25de29a74429de50d11b39bb60e6d7 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Wed, 2 Nov 2016 14:14:48 -0500 Subject: [PATCH 09/11] Remove ProxyStateSet sleeping We no longer need to sleep inside ProxyStateSet because we're directly calling lmgmt->startProxy instead of setting the run_proxy flag and waiting for the TM thread to kick off the lmgmt->startProxy call. --- cmd/traffic_manager/traffic_manager.cc | 2 -- mgmt/LocalManager.cc | 1 - mgmt/LocalManager.h | 1 - mgmt/api/CoreAPI.cc | 12 +++--------- 4 files changed, 3 insertions(+), 13 deletions(-) diff --git a/cmd/traffic_manager/traffic_manager.cc b/cmd/traffic_manager/traffic_manager.cc index b8f4d12f65e..cffbffc73da 100644 --- a/cmd/traffic_manager/traffic_manager.cc +++ b/cmd/traffic_manager/traffic_manager.cc @@ -805,14 +805,12 @@ main(int argc, const char **argv) } else { sleep_time = 1; } - lmgmt->coreapi_sleep = false; if (ProxyStateSet(TS_PROXY_ON, TS_CACHE_CLEAR_NONE) == TS_ERR_OKAY) { just_started = 0; sleep_time = 0; } else { just_started++; } - lmgmt->coreapi_sleep = true; } else { /* Give the proxy a chance to fire up */ just_started++; } diff --git a/mgmt/LocalManager.cc b/mgmt/LocalManager.cc index 50eae96cd9f..9e080a3cebc 100644 --- a/mgmt/LocalManager.cc +++ b/mgmt/LocalManager.cc @@ -189,7 +189,6 @@ LocalManager::LocalManager(bool proxy_on) : BaseManager(), run_proxy(proxy_on), proxy_launch_outstanding = false; mgmt_shutdown_outstanding = MGMT_PENDING_NONE; proxy_running = 0; - coreapi_sleep = true; RecRegisterStatInt(RECT_NODE, "proxy.node.proxy_running", 0, RECP_NON_PERSISTENT); diff --git a/mgmt/LocalManager.h b/mgmt/LocalManager.h index 1c96b22471c..6d884233540 100644 --- a/mgmt/LocalManager.h +++ b/mgmt/LocalManager.h @@ -90,7 +90,6 @@ class LocalManager : public BaseManager bool processRunning(); bool clusterOk(); - volatile bool coreapi_sleep; volatile bool run_proxy; volatile time_t manager_started_at; volatile time_t proxy_started_at; diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index 88da63dce62..49b95ba22c0 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -188,18 +188,12 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear) lmgmt->run_proxy = true; lmgmt->listenForProxy(); - lmgmt->startProxy(tsArgs); - - if (lmgmt->coreapi_sleep) { - do { - mgmt_sleep_sec(1); - } while (i++ < 20 && (lmgmt->proxy_running == 0)); - - if (!lmgmt->processRunning()) - goto Lerror; + if (!lmgmt->startProxy(tsArgs)) { + goto Lerror; } break; + default: goto Lerror; } From 3619a85f9ca5426a65524f9683de41e1dd483331 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Wed, 2 Nov 2016 14:18:28 -0500 Subject: [PATCH 10/11] Code formatting cleanup --- mgmt/LocalManager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mgmt/LocalManager.cc b/mgmt/LocalManager.cc index 9e080a3cebc..a9d692a5418 100644 --- a/mgmt/LocalManager.cc +++ b/mgmt/LocalManager.cc @@ -912,7 +912,7 @@ LocalManager::startProxy(const char *onetime_options) } // Make sure we're starting the proxy in mgmt mode - if (!strstr(proxy_options, MGMT_OPT) && !strstr(onetime_options, MGMT_OPT)) { + if (strstr(proxy_options, MGMT_OPT) == 0 && strstr(onetime_options, MGMT_OPT) == 0) { real_proxy_options.append(" ", strlen(" ")); real_proxy_options.append(MGMT_OPT, sizeof(MGMT_OPT) - 1); } From 9e09c305bd8d707e139173e6493042a9e3be59fd Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Fri, 4 Nov 2016 11:32:31 -0500 Subject: [PATCH 11/11] Fix build warning --- mgmt/api/CoreAPI.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc index 49b95ba22c0..214dc974af7 100644 --- a/mgmt/api/CoreAPI.cc +++ b/mgmt/api/CoreAPI.cc @@ -153,7 +153,6 @@ ProxyStateGet() TSMgmtError ProxyStateSet(TSProxyStateT state, TSCacheClearT clear) { - int i = 0; char tsArgs[MAX_BUF_SIZE]; char *proxy_options;